Closed jeanconn closed 1 year ago
@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?
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).
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).
Updated to use local date-based selection.
"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.
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.
@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?
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.
I think I still need @taldcroft to either agree this is done or decide that #418 is reasonable and should also be merged.
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.
👍 And I confirm I've re-reviewed the diff outputs and see only the expected changes from dynamic bonus.
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
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