Use dyn_bgd_n_faint = 2 #412

jeanconn · 2023-06-16T16:11:58Z

Description

Update starcheck to apply dyn_bgd_n_faint bonus = 2 to guide_count and imposter checking.

Interface impacts

None.

Testing

Tested with ska3-matlab-2023.4rc6 on fido. This PR requires sparkles 4.22.1 .

Unit tests

Unit test added to this as Add a unit test for dyn bgd hotpix imposter calc #418

Functional tests

Ran a set of weeks and tested the cmdline option with output from this script at
https://icxc.cfa.harvard.edu/aspect/test_review_outputs/starcheck-pr412

jeanconn-fido> more run.sh
# Long week and IR Zone holds
/home/jeanconn/git/starcheck/sandbox_starcheck -dir /data/mpcrit1/mplogs/2022/DEC2622/oflsa -out dec2622a_test
/proj/sot/ska3/flight/bin/starcheck -dir /data/mpcrit1/mplogs/2022/DEC2622/oflsa -out dec2622a_flight
/proj/sot/ska/bin/diff2html dec2622a_flight.txt dec2622a_test.txt > dec2622a_diff.html

# Monitor star
/home/jeanconn/git/starcheck/sandbox_starcheck -dir /data/mpcrit1/mplogs/2022/NOV2822/oflsa -out nov2822a_test
/proj/sot/ska3/flight/bin/starcheck -dir /data/mpcrit1/mplogs/2022/NOV2822/oflsa -out nov2822a_flight
/proj/sot/ska/bin/diff2html nov2822a_flight.txt nov2822a_test.txt > nov2822a_diff.html

# Maneuver only loads
/home/jeanconn/git/starcheck/sandbox_starcheck -dir /data/mpcrit1/mplogs/2022/OCT2422/oflsb -out oct2422b_test
/proj/sot/ska3/flight/bin/starcheck -dir /data/mpcrit1/mplogs/2022/OCT2422/oflsb -out oct2422b_flight
/proj/sot/ska/bin/diff2html oct2422b_flight.txt oct2422b_test.txt > oct2422b_diff.html

# Replan
/home/jeanconn/git/starcheck/sandbox_starcheck -dir /data/mpcrit1/mplogs/2021/MAY0521/oflsa -out may0521a_test
/proj/sot/ska3/flight/bin/starcheck -dir /data/mpcrit1/mplogs/2021/MAY0521/oflsa -out may0521a_flight
/proj/sot/ska/bin/diff2html may0521a_flight.txt may0521a_test.txt > may0521a_diff.html

# JUN1923 week Needs dynamic bonus
/home/jeanconn/git/starcheck/sandbox_starcheck -dir /data/mpcrit1/mplogs/2023/JUN1923/oflsa -out jun1923a_test
/proj/sot/ska3/flight/bin/starcheck -dir /data/mpcrit1/mplogs/2023/JUN1923/oflsa -out jun1923a_flight
/proj/sot/ska/bin/diff2html jun1923a_flight.txt jun1923a_test.txt > jun1923a_diff.html

# JUN0523 week Needs dynamic bonus
/home/jeanconn/git/starcheck/sandbox_starcheck -dir /data/mpcrit1/mplogs/2023/JUN0523/oflsa -out jun0523a_test
/proj/sot/ska3/flight/bin/starcheck -dir /data/mpcrit1/mplogs/2023/JUN0523/oflsa -out jun0523a_flight
/proj/sot/ska/bin/diff2html jun0523a_flight.txt jun0523a_test.txt > jun0523a_diff.html

jeanconn · 2023-06-21T13:38:56Z

@taldcroft I was thinking about updating this WIP to set dyn_bgd_n_faint to 2 by default but include a starcheck cmdline option (mostly if we needed to run with dyn_bgd_n_faint = 0 for transition weeks or regression).

What are your thoughts? I think we don't need to take the dyn_bgd_n_faint value from the pickle, though if we did we'd then want to check it against 2 anyway and make sure that dyn_bgd_n_faint wasn't set to an unexpected value of != 2.

What were you thinking on this topic?

taldcroft · 2023-07-04T11:51:07Z

This is a good example of where our infrastructure does not handle changing configurations very well. We need some data source to tell starcheck what is the expected value of dyn_bgd_n_faint and dyn_bgd_dt_ccd at a particular time. chandra_models is an obvious candidate.

Overall I don't think that dyn_bgd_n_faint should be singled out for a new command line option over the many other review criteria that are hardwired. The current situation is that for obsids after the patch uplink the review should be done using 2, -4.0 respectively since those are the flight-approved values of those constants (approved by SSAWG) that reflect the spacecraft capabilities. For the time being I'd prefer setting those constants based on the observation date being before or after the patch uplink date.

I agree with the concept of exporting values like dyn_bgd_n_faint, but not to the top of the starcheck output. It should go to a machine-readable JSON along with all of the other header info. That's a different PR, so for now I'd just remove that new line of starcheck header. (Again, I don't see dyn_bgd_n_faint being conceptually different from the T_ccd limit or any other review critieria. It is just the parameter du jour).

jeanconn · 2023-07-04T12:25:49Z

I just figured a cmdline option was the easiest way to let me see "what would this be without the bonus" which I thought could have value for load review to me and testing during the transition. (Well.. and I thought it was likely that an override to dyn_bgd_n_faint = 3 might be useful).

jeanconn · 2023-07-04T13:16:42Z

Updated to use local date-based selection.

taldcroft · 2023-07-04T13:31:18Z

"what would this be without the bonus"

I'm not sure why we need to answer that question going forward. The program has agreed that mission loads shall be planned using the bonus when dyn bgd is enabled and it is extremely unlikely that we'll ever plan observations using the legacy background.

In the event that we do need to answer that question, it can be done easily enough with a local dev version with changed constants.

starcheck/utils.py

starcheck/src/lib/Ska/Starcheck/Obsid.pm

jeanconn · 2023-07-06T02:14:09Z

And while I'd rerun the testing with the latest updates, I'd forgotten to update the description to indicate that the cmdline option is gone and the starcheck text output is gone. Done.

taldcroft

Finished review. There is only one substantive comment that might require re-running tests, but if so then you might as well address the code nitpicks.

starcheck/src/lib/Ska/Starcheck/Obsid.pm

taldcroft · 2023-07-10T13:14:38Z

starcheck/src/lib/Ska/Starcheck/Obsid.pm

-    # Pass 1 to _guide_count as third arg to use the count_9th mode
-    my $bright_count = sprintf("%.1f",
-        call_python("utils._guide_count", [ \@mags, $self->{ccd_temp}, 1 ]));
+    # Pass 1 to _guide_count as fourth arg to use the count_9th mode


This comment is no longer accurate but can be removed now with the kwargs explicitly called out.

I updated the comment.

starcheck/utils.py

taldcroft · 2023-07-10T13:45:22Z

@jeanconn - looks good. Is there any way in the functional testing to validate the new code which applies the bonus temperatures to the dark cal? From what I can see the code looks reasonable but still. I see you put the t_ccd into a diagnostic dict, so is that exposed somewhere to view?

jeanconn · 2023-07-10T16:17:33Z

Good point on the functional testing. I think I just did some print statements when I was adding the code - I will look to see if it makes sense to grab t_ccd from that diagnostic dict or just print out some stuff for this PR.

jeanconn · 2023-07-11T00:25:23Z

I think I still need @taldcroft to either agree this is done or decide that #418 is reasonable and should also be merged.

jeanconn · 2023-07-11T08:51:05Z

Incremental updates to the testing outputs in the description are non-trivial across an agasc supplement update weekend, so I've set the outputs to be remade and will rereview.

Co-authored-by: Tom Aldcroft <taldcroft@gmail.com>

taldcroft

Looks great!

jeanconn · 2023-07-11T12:17:12Z

👍 And I confirm I've re-reviewed the diff outputs and see only the expected changes from dynamic bonus.

Add a unit test for dyn bgd hotpix imposter calc

One possible quick implementation of bonus

b37be26

jeanconn added 2 commits June 26, 2023 14:08

Use sparkles get_t_ccds_bonus

7e38517

Add cmdline option and output record of dyn_bgd_n_faint

f987d0d

jeanconn changed the title ~~WIP: One possible quick implementation of bonus~~ Add dyn_bgd_n_faint (default=2) to starcheck review Jun 27, 2023

jeanconn requested a review from taldcroft June 27, 2023 17:24

jeanconn changed the title ~~Add dyn_bgd_n_faint (default=2) to starcheck review~~ Add dyn_bgd_n_faint (default=2) to starcheck guide_count Jun 29, 2023

jeanconn added 3 commits July 4, 2023 08:41

Downgrade guide mag warning in general to orange

008fc1e

Use dyn bonus to calculate imposter spoiling

52eaae0

Update comments

cacb54b

jeanconn changed the title ~~Add dyn_bgd_n_faint (default=2) to starcheck guide_count~~ Add dyn_bgd_n_faint (default=2) to starcheck review Jul 4, 2023

Remove dyn_bgd_n_faint cmdline option and printing

b000e4a

jeanconn changed the title ~~Add dyn_bgd_n_faint (default=2) to starcheck review~~ Use dyn_bgd_n_faint = 2 Jul 4, 2023

taldcroft reviewed Jul 4, 2023

View reviewed changes

starcheck/utils.py Outdated Show resolved Hide resolved

starcheck/utils.py Outdated Show resolved Hide resolved

starcheck/utils.py Outdated Show resolved Hide resolved

starcheck/utils.py Outdated Show resolved Hide resolved

starcheck/src/lib/Ska/Starcheck/Obsid.pm Show resolved Hide resolved

Address comments

42f9755

jeanconn requested a review from taldcroft July 10, 2023 13:07

taldcroft reviewed Jul 10, 2023

View reviewed changes

javierggt mentioned this pull request Jul 10, 2023

Release ska3-flight 2023.3 sot/skare3#1089

Closed

41 tasks

jeanconn added 3 commits July 10, 2023 12:56

Rename local functions and address comments

59c16a2

Remove unused params in docstring

9dc6dbb

Add date of dark cal to debug imposter dict

6a7fdbd

jeanconn mentioned this pull request Jul 10, 2023

Add a unit test for dyn bgd hotpix imposter calc #418

Merged

2 tasks

Add a unit test for the dyn bgd hot pixel imposter calc

82750bd

jeanconn and others added 4 commits July 11, 2023 07:46

Fix typo on comparison of dark_date in test

8a1c97d

Co-authored-by: Tom Aldcroft <taldcroft@gmail.com>

Move more testing into the loop comparison

0274bd3

Co-authored-by: Tom Aldcroft <taldcroft@gmail.com>

Cut comparisons done elsewhere

7213d24

Co-authored-by: Tom Aldcroft <taldcroft@gmail.com>

Cut comparison done in the main loop

21e00d8

Co-authored-by: Tom Aldcroft <taldcroft@gmail.com>

taldcroft approved these changes Jul 11, 2023

View reviewed changes

jeanconn added 2 commits July 11, 2023 11:03

Update regex to have 'r'

0ec8d7d

Merge pull request #418 from sot/hotpix-unit-test

6a4b4dd

Add a unit test for dyn bgd hotpix imposter calc

jeanconn merged commit 065d024 into master Jul 11, 2023

jeanconn deleted the bonus branch July 11, 2023 15:09

This was referenced Jul 11, 2023

Update sot/starcheck to 14.1.0 sot/skare3#1115

Closed

2023.3 sot/skare3#1088

Merged

jeanconn mentioned this pull request Jul 21, 2023

Handle vehicle mode with custom filtered backstop #421

Merged

2 tasks

javierggt mentioned this pull request Jul 21, 2023

Update sot/starcheck to 14.1.0 sot/skare3#1134

Closed

javierggt mentioned this pull request Aug 9, 2023

2023.5 sot/skare3#1130

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use dyn_bgd_n_faint = 2 #412

Use dyn_bgd_n_faint = 2 #412

jeanconn commented Jun 16, 2023 •

edited

Loading

jeanconn commented Jun 21, 2023

taldcroft commented Jul 4, 2023

jeanconn commented Jul 4, 2023 •

edited

Loading

jeanconn commented Jul 4, 2023 •

edited

Loading

taldcroft commented Jul 4, 2023

jeanconn commented Jul 6, 2023

taldcroft left a comment

taldcroft Jul 10, 2023

jeanconn Jul 10, 2023

taldcroft commented Jul 10, 2023

jeanconn commented Jul 10, 2023

jeanconn commented Jul 11, 2023

jeanconn commented Jul 11, 2023

taldcroft left a comment

jeanconn commented Jul 11, 2023

Use dyn_bgd_n_faint = 2 #412

Use dyn_bgd_n_faint = 2 #412

Conversation

jeanconn commented Jun 16, 2023 • edited Loading

Description

Interface impacts

Testing

Unit tests

Functional tests

jeanconn commented Jun 21, 2023

taldcroft commented Jul 4, 2023

jeanconn commented Jul 4, 2023 • edited Loading

jeanconn commented Jul 4, 2023 • edited Loading

taldcroft commented Jul 4, 2023

jeanconn commented Jul 6, 2023

taldcroft left a comment

Choose a reason for hiding this comment

taldcroft Jul 10, 2023

Choose a reason for hiding this comment

jeanconn Jul 10, 2023

Choose a reason for hiding this comment

taldcroft commented Jul 10, 2023

jeanconn commented Jul 10, 2023

jeanconn commented Jul 11, 2023

jeanconn commented Jul 11, 2023

taldcroft left a comment

Choose a reason for hiding this comment

jeanconn commented Jul 11, 2023

jeanconn commented Jun 16, 2023 •

edited

Loading

jeanconn commented Jul 4, 2023 •

edited

Loading

jeanconn commented Jul 4, 2023 •

edited

Loading