SHINY: support migration from Chandra.cmd_states / Ska.ParseCM to kadi #30
Conversation
| @@ -35,6 +36,9 @@ | |||
| "less_equal": "<="} | |||
|
|
|||
|
|
|||
| use_noon_day_start() | |||
There was a problem hiding this comment.
This needs a comment, like: Without using this then the epoch times in the existing model spec files are interpreted using midnight for YYYY:DDD dates while the regression data are for YYYY:DDD:12:00:00. This compatibility shim makes everything work without changing all the configured model spec files.
| @@ -499,7 +503,8 @@ def write_states(self, outdir, states): | |||
| states_table['pitch'].format = '%.2f' | |||
| states_table['tstart'].format = '%.2f' | |||
| states_table['tstop'].format = '%.2f' | |||
| states_table.write(outfile, format='ascii', delimiter='\t', overwrite=True) | |||
| states_table.write(outfile, format='ascii', delimiter='\t', overwrite=True, | |||
There was a problem hiding this comment.
Also needs a comment. The fast writer fails because there is an object column (the transition keys) which makes it crash. The Python writer just takes the str of every entry and that gives the desired output.
taldcroft
changed the title
|
Ready for review (along with acisops/backstop_history#9), but I'm not in any particular hurry on this. |
|
@taldcroft thanks for doing this--I started on it but you've gone way beyond what I was anticipating. This week is bad (vacation week for preschool) but I am going to look at this soon. |
| @@ -1009,7 +1014,7 @@ def _setup_proc_and_logger(self, args): | |||
| config_logging(args.outdir, args.verbose) | |||
|
|
|||
| # Store info relevant to processing for use in outputs | |||
| proc = dict(run_user=os.environ['USER'], | |||
| proc = dict(run_user=getpass.getuser(), | |||
There was a problem hiding this comment.
The USER environment variable doesn't exist on Mac. This version is platform-independent.
| states[0].datestart = DateTime(states[0].tstart).date | ||
| states[-1].tstop = DateTime(datestop).secs + 0.01 | ||
| states[-1].datestop = DateTime(states[-1].tstop).date | ||
| dt = 0.01 / 86400 |
There was a problem hiding this comment.
I'm just curious if this is still really needed. Do you have a regression test case where this was required?
There was a problem hiding this comment.
This has been there for eons. Not sure if we really need it or not.
| @@ -29,26 +38,6 @@ def get_prediction_states(self, tlm): | |||
| """ | |||
| raise NotImplementedError("'StateBuilder should be subclassed!") | |||
|
|
|||
| def _get_bs_cmds(self): | |||
There was a problem hiding this comment.
Somewhat gratuitous, but I moved this method back into SQLStateBuilder because it isn't called by ACISStateBuilder and it made me confused to have this as a base class.
|
@taldcroft how can I get an environment on my laptop to test this? I have a fully updated (I think) version of shiny, but I then find that I have to pip install What is the appropriate way to update shiny? I did this: |
|
@taldcroft never mind, I am trying a fresh install of shiny and I'll get back to you. |
|
Hi @taldcroft, Ok, so I managed to get this and the corresponding PR for I ran the regression tests for This is a pretty small error, but I think I'd like to understand where it's happening (maybe it's related to your differences you had above, I'm not sure). This is for the tests using the "SQL" state builder. The ACIS state builder just fails for a reason I need to dig into further. |
|
Some comments:
|
|
Hi @taldcroft, Can you do this run and check the differences in vs. The "SQL" run is missing the ACIS |
|
@jzuhone - thanks for the feedback, very useful! The problem you showed is fixed with 36fdadd, which showed that my understanding of overlapping loads was incomplete. I thought it was just a single AOACRSTD but for MAR0617 there are a couple of subfunction disables after that. Please have a look and see if you think the logic is OK. At some level it is just a matter of getting it to pass regressions because going forward we have RLTT commanding in the loads, so the new logic will never be hit. |
|
@taldcroft I did some more digging into the regression testing of this PR (combined with acisops/backstop_history#9) and I can confirm that both the "SQL" state builder (which we should probably rename to "Kadi" or something similar now) and the "ACIS" state builder give the same results for the DPA model. I haven't checked the other models yet but I suspect I will get a similar answer. Then I checked the new If I compare the "ACIS"
There are some other oddities but I figured this was enough to start--maybe some of these are expected, but I have no idea what is going on with the JUL2717A case. |
|
@taldcroft any way I can help further with this? if you point me in the right direction regarding my last comment about the state differences I might be able to tease it out myself. |
|
@jzuhone - I'll have a look. |
|
FYI, I haven't gotten to this yet due to other priorities but it is still on a sticky on my home page. |
|
@jzuhone -
This is the return-to-science load after the 2017:066 NSM. From what I can see in emails and the CAPs iFOT database, there was not an ECS measurement, which leads me to believe that the correct answer for the My conclusion here is that the "gold standard" values do not correspond to the actual as-flown state of the spacecraft, BUT I could be missing something. The shiny version of continuity state is driven by the commands database which shows: |
|
@taldcroft I can check. What about my second example? That's the really weird one to me. |
|
I'm going one at a time. 😄 |
|
And I assume in this thread that dpa_test_old "flight" is using the ACIS state builder even if that wasn't in use at the time of some of these historical schedules? |
|
@jeanconn that's correct, but the other state builder also gives the same answer. |
|
Thanks, good to know. |
Good to get some real-world testing of kadi states. This was a real bug related to starting the states request mid-maneuver. There was actually a test of exactly this situation (I did worry about this case) but it turned out the test did not hit this bug for obscure reasons. Anyway this is fixed in sot/kadi#176 and I confirmed that now the output states look reasonable. There are still a number of states that only appear in the "old" states file, but these are superfluous since they all occur before the run start time. This might be related to the availability of telemetry at the time it was run or it might be related to how Chandra.cmd_states goes back in time a bit further. In any case @jzuhone, I will put the new version of kadi into shiny and you can update that package and give it another go. And finally I will look at the 3rd point you mentioned. |
This goes away with the latest version of |
|
@taldcroft thanks! I'll update and check things over here and let you know. |
|
@jzuhone - I've updated the shiny package repository with the new version and updated the installation on HEAD. Locally: |
|
Hi @taldcroft, Ok, I've looked at everything for our 4 production models and I am satisfied now with the results. This is good to go. |
Description
This makes necessary changes to migrate from using
Chandra.cmd_statesandSka.ParseCMto just usingkadi.commandsandkadi.commands.states.This is a WIP and requires the ska3 "shiny" distribution (https://github.com/sot/skare3/wiki/Shiny-Ska3). At this moment the shiny distribution is not available for installation and testing, so this PR is just to let ACIS know I'm working on this. After taking a look at the original code I realized that it would probably be a tough job for someone else to do this migration, and in fact I ended up making a number of substantive changes to kadi to support this.
Of course this will need comprehensive testing which is best done by ACIS.
Apologies for all the whitespace changes. VS code just does this for me (extraneous whitespace not allowed by astropy), but it is easy enough to change the GitHub diff settings to ignore whitespace (there is a settings button on the file diffs page, google if you can't find it).
This is paired with a PR on
backstop_history, to be submitted shortly.In addition, this fixes what appears to be bug in calculating the MD5 sum for a thermal model spec. It seems to be computing the MD5 sum on the file name string, not the file contents.
Testing
Functional testing
This is not intended to be comprehensive but just a check that this PR is on the right track.
Using "sql" state builder
On my Mac using the ska3-shiny distribution:
The output states are the same except for the first one.
kadistates always start at exactly the requested start time, not the start of the state it found.The output predicted temperatures are the exactly the same (when rounded to 0.01 degC) except for a startup transient that I believe related to the flight version starting 2 hours earlier (09:32 instead of 11:36).
Using "ACIS" state builder
Run on my Mac. This uses an
sshfsmount to see the appropriate files on the HEAD LAN.The results are similar to the SQL builder diffs: