Handle vehicle mode with custom filtered backstop #421
Conversation
|
@javierggt and @taldcroft I think Tom's idea here to make our own vehicle backstop is workable. I just also think it needs a bunch of testing. But please take a look and let me know if you see fundamental issues with the code. |
| commands are useful for ACA to associate maneuvers with observations. | ||
| """ | ||
| # Use parse_cm read_backstop_as_list instead of kadi.commands.read_backstop | ||
| # as we want the params to keep the SCS to write back later. |
There was a problem hiding this comment.
@taldcroft as I was in a bit of a hurry I didn't dig much into this, but to me it made more sense to use the validated and tagged read-in backstop instead of a grep of some kind in the file. But then if writing out a backstop, it seemed to make sense to use write_backstop... which seemed to have the params it wanted if I used parse_cm's own methods for reading backstop instead of the kadi versions.
|
One of the first things I note, is that when using Part of original dither cmd Re-written cmd |
|
With regard to review of the filtered backstop vs the vehicle backstop in products, the filtered backstop also can apparent display different VCDU values for ORBPOINT events. It looks like when generating vehicle backstop these are set to the value of the last command before the ORBPOINT -- in our new filtered backstop they are just copied over from their values in the combined backstop. This seems completely moot and I think ORBPOINT should probably have VCDU 0 like the LOAD_EVENT cmds but... |
| @@ -543,3 +544,20 @@ def proseco_probs(**kw): | |||
| ] | |||
|
|
|||
| return p_acqs, float(-np.log10(acq_cat.calc_p_safe())), float(np.sum(p_acqs)) | |||
|
|
|||
|
|
|||
| def vehicle_filter_backstop(backstop_file, outfile): | |||
There was a problem hiding this comment.
Ideally, we'd add a unit test for this function, but given the impact I think it can probably go with functional testing.
|
This looks good from my first look. For unit-ish testing, at a bare minimum you should use |
|
Sounds good for the bare minimum unit-ish testing. And as you can tell from my comments that's the first thing I was looking at in my initial functional review. It looks like it doesn't quite round trip so far with the removal of the comma-space-comma text. |
That comma-space-comma seems like a DS bug in OR-list writing, but in any case that will be an acceptable diff. |
jeanconn
changed the title
|
I've removed the "WIP" for this PR and updated the description to document that regression-oriented functional testing that I did to look this over. |
Handle vehicle mode with custom filtered backstop (was PR #421)
Description
Handle vehicle mode with custom filtered backstop
Fixes issue that -vehicle mode does not run after #408
Interface impacts
In the case of "-vehicle" option, makes a new filtered backstop file in the output directory that is read in for all other command checks.
Testing
Unit tests run out of ska3-flight-2023.3rc8 on OSX
Unit tests
Independent check of unit tests by [REVIEWER NAME]
Functional tests
Testing was run with the /proj/sot/ska3/test/ environment set to 2023.3rc8 from fido.
Testing confirming new starcheck vehicle filtered backstop is reasonable.
For this, used a hybrid approach and used grep to remove science-only scs cmds and sed to filter the comma-space-comma issue on combined backstop -- and then diffed with the filtered backstop from this PR. The diffs show the expected value that MP_OBSID commands are the only additional things included in our filtered backstop from this PR.
This script and outputs are in https://icxc.cfa.harvard.edu/aspect/test_review_outputs/starcheck-pr421/compare_vbackstop_to_products/
Testing confirming there are no differences to the 14.0.1 release for the combined products
(this PR only changes vehicle outputs). The diffs are as-expected.
This script and outputs are in https://icxc.cfa.harvard.edu/aspect/test_review_outputs/starcheck-pr421/compare_vs_14.1.0/
Confirm that diffs between combined and vehicle outputs look reasonable for this PR
Outputs in https://icxc.cfa.harvard.edu/aspect/test_review_outputs/starcheck-pr421/vehicle_vs_combined
Compare output of this PR to current flight
Outputs in https://icxc.cfa.harvard.edu/aspect/test_review_outputs/starcheck-pr421/compare_vs_flight
This show the expected diffs due to several PRs, including all of the small timing differences due to #408 (use calculated maneuvers instead of maneuver summary) , the updates for #412 (apply dynamic background bonus), and #413 (use expanded high-IR zone). I tried to reduce diffs a bit by setting the new run of starcheck to use chandra_models 3.48 with the previous version of the acquisition model.
The diffs show the expected small timing differences in maneuver end time for all observations. Additionally, in the vehicle diffs there are tiny timing differences on the reference time used for gyro hold observations -- It looks like the time of the MP_OBSID command in the new filtered vehicle backstop ends up as the reference time for these observations in this new approach. Previously it looks like they were assigned the time of the AOUPTARG command as the reference time. This is benign for these no-starcat observations.