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

jeanconn · 2023-10-17T23:39:27Z

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 sot/chandra_aca#157

Unit tests

Mac

Independent check of unit tests by @taldcroft

Mac

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

I know this is a WIP but it looks really good and just needs a few changes and some testing. We should go ahead with this soon!

proseco/fid.py

taldcroft · 2023-10-23T13:15:05Z

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:

"True": always apply and require that t_ccd and date are supplied
"False": never apply
None (undefined, which will be the default): apply if t_ccd and date are both supplied.

jeanconn · 2023-10-23T13:25:07Z

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

jeanconn · 2023-10-30T17:45:38Z

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

jeanconn · 2023-11-01T14:32:38Z

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 · 2023-11-01T15:19:36Z

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 · 2023-12-05T10:05:21Z

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

pr388-backstop-vs-proseco-star-positions.ipynb
pr388-validate-drift-model-fid-positions.ipynb

docs/index.rst

proseco/fid.py

proseco/tests/test_fid.py

Co-authored-by: Tom Aldcroft <[email protected]>

jeanconn · 2023-12-08T19:33:56Z

proseco/core.py

@@ -1487,7 +1487,8 @@ def add_fake_stars_from_fid(
                detector=detector,
                sim_offset=sim_offset,
                focus_offset=0,
-                t_ccd=-10.0,
+                t_ccd_guide=-10.0,
+                t_ccd_acq=-10.0,


This may be controversial, but it seems to be in the context of use in a test function that is in proseco.core (maybe should be somewhere in tests?).

Can reverting this count as the test of changing the t_ccd setter?

I've just un-done this (makes sense at this point), though did your comment mean that you wanted me to remove some of my gratuitous t_ccd tests?

proseco/fid.py

jeanconn · 2023-12-12T19:53:03Z

proseco/fid.py

@@ -128,6 +131,7 @@ def t_ccd(self):
    @t_ccd.setter
    def t_ccd(self, value):
        self.t_ccd_guide = value
+        self.t_ccd_acq = value


In more detailed review and testing, this was the change needed so that "get_fid_catalog" will mostly do the right thing if just a t_ccd is supplied. In that case the temperature is assigned to both t_ccd_guide and t_ccd_acq and fid offsets will be applied based on t_ccd_acq by default. This was already the default behavior for get_aca_catalog, but not for get_fid_catalog.

Seems reasonable.

proseco/tests/test_common.py

taldcroft · 2023-12-13T13:08:02Z

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 · 2023-12-13T15:20:05Z

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.

taldcroft

@jeanconn - I pushed a final commit with some cleanup. I re-reviewed the key validation notebook and code. It all looks good now, thanks for this important fix!

taldcroft · 2023-12-14T11:05:03Z

proseco/tests/test_catalog.py

+
+    aca_args = STD_INFO.copy()
+    for kw in ["t_ccd", "t_ccd_acq", "t_ccd_guide"]:
+        if kw in aca_args:


For future reference, a one-line way to do this is aca_args.pop(kw, None).

And if it ever comes up again, we could update mod_info() (or whatever is that function) to allow for deleting kwargs via mod_info(delete_kwargs=("t_ccd", "t_ccd_acq", "t_ccd_guide")).

taldcroft · 2023-12-14T11:07:48Z

proseco/tests/test_catalog.py

    }


+def test_t_ccd_attr():


I like the new tests!

jeanconn added 2 commits October 13, 2023 11:08

Implement use of an initial fid drift correction

0c86b27

Update to use new get_fid_offset function in chandra_aca

f39da1e

taldcroft mentioned this pull request Oct 23, 2023

Add too-cold check for fid lights at acquisition sot/sparkles#199

Closed

2 tasks

taldcroft reviewed Oct 23, 2023

View reviewed changes

proseco/fid.py Outdated Show resolved Hide resolved

proseco/fid.py Outdated Show resolved Hide resolved

proseco/fid.py Outdated Show resolved Hide resolved

taldcroft reviewed Oct 23, 2023

View reviewed changes

proseco/fid.py Outdated Show resolved Hide resolved

proseco/fid.py Outdated Show resolved Hide resolved

Update PROSECO_ENABLE_FID_OFFSET to tri-state

c29342b

jeanconn changed the title ~~WIP: Apply offset to fid positions~~ Apply offset to fid positions Oct 29, 2023

Update warning text

e52fe59

jeanconn marked this pull request as ready for review October 29, 2023 16:18

jeanconn marked this pull request as draft November 1, 2023 17:36

jeanconn marked this pull request as ready for review November 1, 2023 17:38

jeanconn added 5 commits November 11, 2023 08:47

Remove unnecessary monkeypatch

550e0ef

Update comment

b4bfa31

Update docs to include new env var

56d76a1

Add fid offset functional testing and position check notebooks

d62bc35

Add a couple of comments to the fid review notebook

5595c4c

jeanconn requested a review from taldcroft November 30, 2023 18:04

taldcroft changed the title ~~Apply offset to fid positions~~ Apply temperature dependent drift model offset to fid positions Dec 5, 2023

taldcroft changed the title ~~Apply temperature dependent drift model offset to fid positions~~ Apply temperature-dependent drift model offset to fid positions Dec 5, 2023

taldcroft reviewed Dec 5, 2023

View reviewed changes

jeanconn and others added 4 commits December 5, 2023 15:43

Update docs/index.rst

af53856

Co-authored-by: Tom Aldcroft <[email protected]>

Update proseco/fid.py in fid position docs

e97c593

Co-authored-by: Tom Aldcroft <[email protected]>

Update proseco/fid.py ValueError text

b37bd20

Co-authored-by: Tom Aldcroft <[email protected]>

Update proseco/fid.py get_fid_position docs

a7325d3

Co-authored-by: Tom Aldcroft <[email protected]>

Update fid positions in exp/regress values to offset positions

2b0e3a1

jeanconn commented Dec 8, 2023

View reviewed changes

jeanconn added 2 commits December 8, 2023 14:36

Remove unused imports

7a299d0

Use a FIDS test fixture

b5fcc08

jeanconn requested a review from taldcroft December 8, 2023 19:58

Rename notebooks

ef377fa

jeanconn commented Dec 8, 2023

View reviewed changes

proseco/fid.py Outdated Show resolved Hide resolved

jeanconn commented Dec 8, 2023

View reviewed changes

proseco/fid.py Show resolved Hide resolved

jeanconn added 7 commits December 8, 2023 16:17

Rerun validation notebook

3761755

Set t_ccd_acq from t_ccd kwarg

1535aef

Add a test fixture to disable fid offsets

9036aa0

Disable fid offsets in two tests

be17b85

Make some new t_ccd kwarg/attr tests

989143d

Rerun validate notebook

e8a5529

Restore previous comment

440b2da

jeanconn commented Dec 12, 2023

View reviewed changes

proseco/tests/test_common.py Show resolved Hide resolved

Use disable_fid_offsets fixture in an acq test

0b63ec0

Revert change to add_fake_stars_from_fid

5c2b26f

jeanconn requested review from taldcroft and removed request for taldcroft December 13, 2023 15:18

Some tiny cleanup

d86e281

taldcroft approved these changes Dec 14, 2023

View reviewed changes

jeanconn merged commit e1c1d0f into master Dec 17, 2023
2 checks passed

jeanconn deleted the fid-offset branch December 17, 2023 20:31

This was referenced Jan 9, 2024

Update sot/proseco to 5.12.0 sot/skare3#1233

Closed

2023.10 sot/skare3#1242

Merged

javierggt mentioned this pull request Feb 6, 2024

2023.11 sot/skare3#1246

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

jeanconn commented Oct 17, 2023 •

edited by taldcroft

Loading

taldcroft left a comment

taldcroft commented Oct 23, 2023

jeanconn commented Oct 23, 2023

jeanconn commented Oct 30, 2023

jeanconn commented Nov 1, 2023

taldcroft commented Nov 1, 2023

taldcroft commented Dec 5, 2023

jeanconn Dec 8, 2023 •

edited

Loading

taldcroft Dec 13, 2023

jeanconn Dec 13, 2023

jeanconn Dec 12, 2023

taldcroft Dec 13, 2023

taldcroft commented Dec 13, 2023

jeanconn commented Dec 13, 2023

taldcroft left a comment

taldcroft Dec 14, 2023

taldcroft Dec 14, 2023

taldcroft Dec 14, 2023

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

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

Conversation

jeanconn commented Oct 17, 2023 • edited by taldcroft Loading

Description

Interface impacts

Testing

Unit tests

Functional tests

taldcroft left a comment

Choose a reason for hiding this comment

taldcroft commented Oct 23, 2023

jeanconn commented Oct 23, 2023

jeanconn commented Oct 30, 2023

jeanconn commented Nov 1, 2023

taldcroft commented Nov 1, 2023

taldcroft commented Dec 5, 2023

jeanconn Dec 8, 2023 • edited Loading

Choose a reason for hiding this comment

taldcroft Dec 13, 2023

Choose a reason for hiding this comment

jeanconn Dec 13, 2023

Choose a reason for hiding this comment

jeanconn Dec 12, 2023

Choose a reason for hiding this comment

taldcroft Dec 13, 2023

Choose a reason for hiding this comment

taldcroft commented Dec 13, 2023

jeanconn commented Dec 13, 2023

taldcroft left a comment

Choose a reason for hiding this comment

taldcroft Dec 14, 2023

Choose a reason for hiding this comment

taldcroft Dec 14, 2023

Choose a reason for hiding this comment

taldcroft Dec 14, 2023

Choose a reason for hiding this comment

jeanconn commented Oct 17, 2023 •

edited by taldcroft

Loading

jeanconn Dec 8, 2023 •

edited

Loading