sot / starcheck

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

Support move to 8x8 readouts for science observations #396

Closed jeanconn closed 1 year ago

jeanconn commented 1 year ago

Description

Support move to 8x8 readouts for science observations.

This includes an update to the ACA checklist to require that science observations are in 8x8 or 6x6 (that seemed reasonable instead of being out of compliance with the checklist during transition).

The starcheck update simply uses PROSECO_OR_IMAGE_SIZE or the new default of '8' to check the size on ORs. The warning about "stealth" monitor windows is just removed as not needed.

Interface impacts

With this change, starcheck will respect the PROSECO_OR_IMAGE_SIZE environment variable to do its checks on ORs.

Testing

Unit tests

Functional tests

I set KADI_SCENARIO to flight until #400 is merged.

ska3-jeanconn-fido> env KADI_SCENARIO='flight' /home/jeanconn/git/starcheck/sandbox_starcheck -dir /data/mpcrit1/mplogs/OFLS_testing/2022/OCT0322/oflst -out test_oct0322t

https://icxc.cfa.harvard.edu/aspect/test_review_outputs/starcheck-pr396/test_oct0322t.html

ska3-jeanconn-fido> env KADI_SCENARIO='flight' starcheck -dir /data/mpcrit1/mplogs/OFLS_testing/2022/OCT0322/oflst -out flight_oct0322t

https://icxc.cfa.harvard.edu/aspect/test_review_outputs/starcheck-pr396/flight_oct0322t.html

DIFF: https://icxc.cfa.harvard.edu/aspect/test_review_outputs/starcheck-pr396/diff_oct0322t.html

ska3-jeanconn-fido> env KADI_SCENARIO='flight' PROSECO_OR_IMAGE_SIZE='6' /home/jeanconn/git/starcheck/sandbox_starcheck -dir /data/mpcrit1/mplogs/2022/OCT0322/oflsa -out test_6_oct0322a

https://icxc.cfa.harvard.edu/aspect/test_review_outputs/starcheck-pr396/test_6_oct0322a.html

ska3-jeanconn-fido> env KADI_SCENARIO='flight' /home/jeanconn/git/starcheck/sandbox_starcheck -dir /data/mpcrit1/mplogs/2022/OCT0322/oflsa -out test_8_oct0322a

https://icxc.cfa.harvard.edu/aspect/test_review_outputs/starcheck-pr396/test_8_oct0322a.html

And after the last commit, I redid the first test output and confirmed no significant diffs:

ska3-jeanconn-fido> diff test_oct0322t.txt test_oct0322t_redo.txt
1,2c1,2
<  ------------  Starcheck 13.16.1.dev2+g2362a95    -----------------
<  Run on Wed Nov  2 15:11:02 EDT 2022 by jeanconn from fido.cfa.harvard.edu
---
>  ------------  Starcheck 13.16.1.dev3+g17cf448    -----------------
>  Run on Thu Nov  3 19:39:44 EDT 2022 by jeanconn from fido.cfa.harvard.edu
jeanconn commented 1 year ago

For a promotion strategy here, do we have interest in setting PROSECO_OR_IMAGE_SIZE to 6x6 in the skare scripts now? Or changing the default in the starcheck code back to 6x6 and toggling it by setting PROSECO_OR_IMAGE_SIZE to 8x8 when we make the transition?

taldcroft commented 1 year ago

The code changes look correct, but I think it can be much simpler by just doing the following right at the check.

my $img_size = $ENV{PROSECO_OR_IMAGE_SIZE} || '8';
my $or_size = "${img_size}x${img_size}";

The way you have written it makes the provenance of $or_size in the checking code really obscure by relying on a global that gets set by code running a different module. You have to do 3 hops to figure out what is going on.

taldcroft commented 1 year ago

There should be mention of the new environment variable in the starcheck -help docs.

jeanconn commented 1 year ago

Good point that this probably doesn't need to be set as a package variable in Obsid.pm. I originally thought we needed to use the $or_size in a bunch of places but trimmed it down and didn't do the final simplifying.

taldcroft commented 1 year ago

About promotion strategy, what about using the proseco version which is captured in the pickle file? I think that >= 5.8.0 would be 8x8 and otherwise 6x6?

jeanconn commented 1 year ago

I mean, we could do that, it just seems like overkill and wouldn't help if we decided to use PROSECO_OR_IMAGE_SIZE on the FOT side.

jeanconn commented 1 year ago

As it is, if this gets approved at the same time as 2022_060 we don't really need to do anything.

jeanconn commented 1 year ago

I consider regression testing a different kettle of fish.

taldcroft commented 1 year ago

I'm not sure we'll manage to exactly synchronize the releases, but you're right that in any case we'll be OK.

taldcroft commented 1 year ago

@jeanconn - I looked over the functional test results and they generally look OK. Do you understand the changes in the Guide summary vs AGASC supplement warnings? I assume that you did the test runs at basically the same time so the supplement should be giving the same answers.

taldcroft commented 1 year ago

Oops, I wrote a little too soon... this link is broken: https://icxc.cfa.harvard.edu/aspect/test_review_outputs/starcheck-pr396/test_8_oct0322a.html

jeanconn commented 1 year ago

Thanks. I had rerun just the test testing yesterday (and apparently missed that test_8 was missing). Flight outputs were older. I'll rerun all for record and ping again for a final check when I've reviewed the outputs. I had previously just ignored all guide summary vs agasc warnings as non-informative but it is a good point that if flight and test are run on the same supplement the warnings should be the same and go away in the diff.

jeanconn commented 1 year ago

I re-ran and reviewed test outputs and they seem good.

javierggt commented 1 year ago

For the record:

I just ran starcheck from this branch (in load review) and compared it with the current flight version and confirmed that the only difference in the output are all the critical warnings.

jeanconn commented 1 year ago

I think you're OK with this @taldcroft but I couldn't find a review status in the history.