Closed jeanconn closed 1 year ago
jeanconn
commented
1 year ago @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.
jeanconn
commented
1 year ago One of the first things I note, is that when using diff to compare the vehicle backstop in products to the new backstop we're making here, in addition to having the MP_OBSID commands, we also see diffs in MP_STARCAT and MP_DITHER commands. At first glance these diffs look benign as they appear to be due to the fact that the re-write of the commands in the new file has removed extra undefined parameters. Specifically, removed , , (comma-space-comma) entries from Backstop.
Part of original dither cmd
RATEY= 5.78052923e-03, , SCS= 130, STEP= 1324Re-written cmd
RATEY= 5.78052923e-03, SCS= 130, STEP= 1324With 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...
taldcroft
commented
1 year ago This looks good from my first look.
For unit-ish testing, at a bare minimum you should use vehicle_filter_backstop to filter a backstop and then show the diff from the original vehicle backstop. The intent was for the read/write backstop functions to round-trip, but I am not sure of it.
jeanconn
commented
1 year ago 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.
taldcroft
commented
1 year ago 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
commented
1 year ago 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.
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.