sot / starcheck

BSD 3-Clause "New" or "Revised" License
3 stars 0 forks source link

Add 16 arcsec to 'standard' dither in y and z #383

Closed jeanconn closed 1 year ago

jeanconn commented 2 years ago

Description

Add 16 arcsec to 'standard' dither in y and z

Interface impacts

None

Testing

Unit tests

Functional tests

Removes "Non-standard dither" CAUTION on obsid 21250 which is 16x16.

ska3-fido$ diff MAY2719A_flight.txt MAY2719A_test.txt
1,2c1,2
<  ------------  Starcheck 13.15.1    -----------------
<  Run on Thu Jun  2 12:09:33 EDT 2022 by jeanconn from fido.cfa.harvard.edu
---
>  ------------  Starcheck 13.15.2.dev0+gb454063.d20220602    -----------------
>  Run on Thu Jun  2 14:51:31 EDT 2022 by jeanconn from fido.cfa.harvard.edu
61c61
< OBSID = 21250 at 2019:149:01:10:05.020   7.8 ACQ | 5.0 GUI | Caution: 4
---
> OBSID = 21250 at 2019:149:01:10:05.020   7.8 ACQ | 5.0 GUI | Caution: 3
747d746
< >> CAUTION : Non-standard dither
taldcroft commented 2 years ago

This PR achieves the stated objective with a minimal footprint.

However, looking at the code, the logic could use improvement. E.g. if dither of 8x20 is passed then it declares that as standard dither for acis. From what I can see the return value (e.g. acis) is not checked, so no warning would be issued even for an HRC observation with 8x8 dither. (HRC review would catch this though).

It's probably worth cleaning up this whole function, probably taking the detector as an argument and always returning a bool (0 or 1).

javierggt commented 1 year ago

I am adding a commit to switch the dither periods, because they are switched in recent loads. I used this branch, including my commit, to review the DEC1922 loads and can confirm that the only change with the current flight review is the disappearance of the non-standard dither warnings (except the three observations with actual non-standard dither).

I also had to rebase on master for things to work, because of window size warnings and high-IR zone checks.

jeanconn commented 1 year ago

With regard to corner cases; the recent 16x8 observations have different periods than I (or this code) expected, but maybe that is still fine to push this as an incremental improvement.