Skip to content

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

Merged
taldcroft merged 2 commits intomasterfrom
dither16
Jan 5, 2023
Merged

Add 16 arcsec to 'standard' dither in y and z#383
taldcroft merged 2 commits intomasterfrom
dither16

Conversation

@jeanconn
Copy link
Copy Markdown
Contributor

@jeanconn jeanconn commented Jun 2, 2022

Description

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

Interface impacts

None

Testing

Unit tests

  • No 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

@jeanconn jeanconn requested a review from taldcroft June 8, 2022 16:38
@taldcroft
Copy link
Copy Markdown
Member

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).

@jeanconn jeanconn changed the title Add 16 arcsec to 'standard' dither in y and z WIP: Add 16 arcsec to 'standard' dither in y and z Jun 8, 2022
@jeanconn jeanconn removed the request for review from taldcroft August 22, 2022 15:15
@javierggt
Copy link
Copy Markdown
Contributor

javierggt commented Dec 15, 2022

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.

Copy link
Copy Markdown
Member

@taldcroft taldcroft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the spirit of "perfect is the enemy of good", let's just run with this. The corner case that I highlighted earlier doesn't happen in practice and even if it did this is still perfectly good for ACA.

@taldcroft taldcroft changed the title WIP: Add 16 arcsec to 'standard' dither in y and z Add 16 arcsec to 'standard' dither in y and z Dec 19, 2022
@jeanconn
Copy link
Copy Markdown
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants