-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use kadi dynamic commanded states for starcheck thermal model #330
Conversation
@taldcroft I think this is all looking good, but what would you like to see as documentation of testing for both the PR and for LR? As we've changed
I'm not sure if each needs to be validated independently or if "acq and guide temps for a set of schedules match expectations" is enough. |
Showing that "acq and guide temps for a set of schedules match expectations" is enough, where of course the set of schedules includes a couple with eclipses. |
Regarding eclipses, I suppose I can use the proseco pkl files for straightforward independent determination of those expectations (I had just been going "oh that's different and probably right due to eclipse"). |
Or use one of the eclipse loads that failed starcheck but passed FOT thermal, and show now passing. I would approve that. |
31b9233
to
6d180c1
Compare
Regarding the states in general, I was happy to see the temperatures matched the MCC/pkl ones much better for the eclipse schedules, though I wasn't sure if we should add eclipse to the starcheck aca thermal model plot. I also thought these were close to the mcc values but not quite as close as I would have guessed. (and I still had an action to try to understand what is happening in the beginning of JUN1719A) |
Re eclipse checking, looks great, I'll call that a pass! |
6d180c1
to
b1bc7d1
Compare
b1bc7d1
to
6be87a5
Compare
Code looks good now, thanks! Just need to re-run the validation plots and standard regression. |
Regarding standard regression, I ran with the "-3" run_start_time vs current master (not release) and get text diffs that look reasonable to me, with temperature differences that look up to 0.1 deg on first scroll-through: |
In more review it looks like there are some larger temperature diffs. I notice some in JAN1215B which should have eclipses in it. I would prefer to do more rigorous eval of the diffs (and confirm that all larger diffs are due to eclipses) but I think basically we are OK and any reasonable diffs related to temperature are fine. We're mostly using the full regression set to just confirm that everything runs and there are no new weird errors. |
For the validation plots, I ran these with these start times
And rerun of the weeks and their plots comparing to the pickle temperatures look the same |
@taldcroft Is this officially good to go now? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent!!
Use kadi dynamic commanded states for starcheck thermal model
Closes #329
Supersedes #319