feat: add v2 pointing model for lat #1448
Conversation
|
This doesn't need a version bump, since the "v2" parameters are a superset of the v1 parameters. There will be less code duplication if we extend the existing function rather than add a whole new almost-the-same one. |
|
Also, wouldn't it be more natural to apply the el right after the encoder el offset, instead of after it's been through a bunch of quaternion operations? |
I think my one issue here is the scenario where someone has the new model but the old code, I don't think that would error out currently without a version bump. I suppose I could instead add a check that there are no unknown pars?
Right now the sequence of operations go sort of "inside-out", so encoder offsets -> LATR offsets -> bearing offsets -> mirror offsets -> telescope tilt + sag. I can be talked out of this ordering (or convinced that tilt + sag is more "inside" than the LATR perhaps). |
I believe it would error out; look for I had this situation in mind. You can even use new model params with old code, as long as the values are zero. |
10 steps ahead as usual, I'll refactor things to be all in v1 and just edit the field in the h5 files. |
|
Updated, @mhasself one thing you may see and object to is that I calculate a corrected az and el and then apply the base tilt and pointing model on that. You may (reasonably) say that I should do that earlier and only calculate a new az and el once, but this current order of operations is what was used when I fit the model. If there is an equivalent (ie: no need to refit) method that is cleaner that is ideal and I am happy to switch over, otherwise I would need to refit which may or may not be worth it (or if I can calculate a conversion that is also fine I suppose). |
sotodlib/coords/pointing_model.py
Outdated
| 'lat_v2' : { | ||
| 'enc_offset_az': 0, | ||
| 'enc_offset_el': 0, | ||
| 'enc_offset_cr': 0, | ||
| 'el_axis_center_xi0': 0, | ||
| 'el_axis_center_eta0': 0, | ||
| 'cr_center_xi0': 0, | ||
| 'cr_center_eta0': 0, | ||
| 'mir_center_xi0': 0, | ||
| 'mir_center_eta0': 0, |
There was a problem hiding this comment.
Delete these -- this is still lat_v1.
As written, I believe the code will error if you pass it an old params dict. Because the updated defaults will not be loaded. A smoke test would be helpful here.
Please add LAT model test to tests/test_pointing_model.py, including backwards compatibility check.
|
@mhasself let me know if these tests are sufficient. I didnt check behavior of all pars, just a basic sanity check that it works in both modes. I do want to try moving the el sag up (right after the encoder offsets), but I want to see if I can translate the already fit params for that rather than doing another fit. |
No description provided.