sot / proseco

Probabilistic star evaluation and catalog optimization
https://sot.github.io/proseco
BSD 3-Clause "New" or "Revised" License
1 stars 0 forks source link

Apply temperature-dependent drift model offset to fid positions #388

Closed jeanconn closed 11 months ago

jeanconn commented 1 year ago

Description

Update proseco to apply the time and temperature-dependent fid position offsets to the fid positions in proseco constructed catalogs. This code relies on chandra_aca.drift.get_fid_offset and the aimpoint drift model in chandra_models.

Interface impacts

Changes default behavior in proseco to apply offsets in y and z to expected fid positions via chandra_aca.drift.get_fid_offset. Respects new environment variable PROSECO_ENABLE_FID_OFFSET .

Testing

Requires https://github.com/sot/chandra_aca/pull/157

Unit tests

Independent check of unit tests by @taldcroft

Functional tests

Added a unit test. Reselected ~900 catalogs in two aca aimpoint drift model epochs and since the last safe modes (2023:044 and 2023:047) and confirmed proseco fid positions with the PR are overall reasonable compared to observed positions and that there are small mean offests since the last safe mode (expected). Also reviewed reselected star catalogs with critical warnings (with and without fid offsets applied) and confirmed proseco catalogs are reasonable (one new critical warning on an hrc fid light in a field with many spoiler stars).

Functional review notebook is part of the PR in the analysis directory https://github.com/sot/proseco/blob/5595c4c002814b82ccee2f20f4f084bd14850770/analysis/reselect_fid_offset.ipynb

taldcroft commented 1 year ago

There is a slight complication here: https://github.com/sot/kadi/blob/76b7d0b6b79102ab60c53df302eab1eb42c65b0e/kadi/commands/observations.py#L85

The current code will break this because t_ccd is not known. This supports a thought I was having independently, which is that if t_ccd and date are both None then that should imply not adding the offset. That also leads to a 3-state value for PROSECO_ENABLE_FID_OFFSET:

jeanconn commented 1 year ago

Those kadi observations are an interesting use case. Thanks for bringing that up.

jeanconn commented 1 year ago

I forgot -- I need to document the functional testing in here so that's still to-do.

jeanconn commented 1 year ago

We probably also need to handle dates < 2012 explicitly (perhaps with a warning instead of rethrowing the valueerror from chandra_aca) if we want to leave this turned on and run old schedules in matlab regression for example.

taldcroft commented 1 year ago

Good point about the 2012 cutoff from this code in chandra_aca.drift:

        if times[0] < DateTime("2012:001:12:00:00").secs:
            raise ValueError("model is not applicable before 2012")

One idea is to just change the drift model to extrapolate back from 2012 with a constant via:

        times = times.clip(DateTime("2012:001:12:00:00").secs, None)
taldcroft commented 11 months ago

One more thing, can you make the notebook names more descriptive. I'd suggest:

taldcroft commented 11 months ago

Is this ready again for review? BTW I put this as Squash and Merge, unless you are adamant that all 36 commits should go in the permanent history.

jeanconn commented 11 months ago

Sure. I just made that change to add_fake_stars_from_fid and reran unit tests (did not re-run functional/notebook). This is ready for review again. And sure I don't care if we keep all the commits if this is approved.