-
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 RLTT and schedule_stop in thermal calculation #344
Conversation
# If the last obsid interval extends over the end of states then | ||
# extend the state / predictions. | ||
last_state = states[-1] | ||
last_sc_obsid = sc_obsids[-1] |
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.
I don't understand the circumstances where starcheck would make an obsid interval that extends beyond the end of commanding. With SCHEDULED_STOP in backstop this bit is not needed, but wondering if it is needed for regression testing or whatnot.
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.
IIRC in the absence of something useful like SCHEDULED_STOP, if the schedule ended in NPNT (and had no maneuver in backstop to define end time), the obsid stop time for the last observation in the schedule was sometimes set from the stop time listed in the processing summary? Maybe?
I remember spending a lot of time on corner cases to get it to not just fail out. Should be moot if we can do a modern hopper transition and better state building but there you go.
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.
I added your text as a comment to the code for our future selves.
Looks like you've also closed #62 for testing. |
|
||
if json_obsids is None: | ||
# Only happens in testing, so use existing obsids file in OFLS dir | ||
json_obsids = Path(oflsdir, 'starcheck', 'obsids.json').read_text() |
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.
I suppose if one uses a local oflsdir one would not need to presume that the tested changes in local starcheck weren't causing diffs in the obsids.json.
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.
I don't understand this.
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.
If we were to change something about the timing of the obsids (saved in that obsids.json file) with a change in starcheck, the new 'test code' here in calc_ccd_temps.py probably wouldn't get the right answer to test a new starhceck release candidate if you use /data/mpcrit1/mplogs oflsdirs for input. I think I was saying, that your test fixes in calc_ccd_temps.py are great but they don't solve all integration testing issues with the Perl pieces of starcheck.pl.
This looks good to me and I ran the regression weeks... but I need to go over the outputs a bit. |
Any progress there? Would be good to get this merged before we've forgotten everything and have to re-review from scratch. |
Not really. I think I just want to rerun the regression outputs just for this change and confirm there are just no changes. I should be able to wrap that up today. Is the issue with timeliness on merging or promoting? I figured this could probably go next week but I don't know if you are done with kadi and/or if we wanted another skare3 release before the namespace one. |
It's just merging. I got behind on reviewing PR's but in general am striving to avoid old-but-active PRs in our work. |
And I'm not done with review. I still don't quite understand changes in the calculated ACA temperatures in the regression loads (as they do not have RLTT or schedule stop). They are small changes but I'm not clear about which piece is introducing them. |
|
||
# Add in the backstop commands | ||
cmds = cmds.add_cmds(bs_cmds) | ||
|
||
# Get the states for available commands. This automatically gets continuity. | ||
state_keys = ['obsid', 'pitch', 'q1', 'q2', 'q3', 'q4', 'eclipse'] | ||
states = kadi_states.get_states(cmds=cmds, state_keys=state_keys, | ||
merge_identical=True) | ||
states = kadi_states.get_states(cmds=cmds, start=init_tlm_time, stop=sched_stop, |
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.
I had been thinking of the get_states changes as just a convenience. Here, now that start
is passed, it looks like the first returned state in the (propagation interval of the) test load I'm viewing (JAN117A), now starts 15 minutes earlier at init_tlm_time
instead of the time of cmds[0]
. I think that is totally fine for starcheck's thermal model but I wasn't expecting the change.
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.
More than fine, it is now correct. Previously the propagation temperature from available telemetry was being applied starting at the wrong time. In most circumstances it didn't really matter, but it is theoretically possible that the first command could be many hours after init_tlm_time
, e.g. during a long observation between comms.
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.
That was my thinking as well (that it is more correct), though I figured it was worth calling out in discussion in the PR as it is a functional change that was not mentioned in the PR top matter.
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.
I'm good with this, with the understanding that the constructed states for propagation now start at init_tlm_time
instead of, what, the first command after init_tlm_time
? Seems reasonable.
Description
This jumps off from #341 and implements the suggestions there, and then makes a number of changes so I could test this. Requires sot/kadi#164.
Testing
Fixes #340
Fixes #62
Functional testing
Used this script:
Outputs