sot / kadi

Chandra commands and events
https://sot.github.io/kadi
BSD 3-Clause "New" or "Revised" License
5 stars 3 forks source link

Fix misorders between obsid cmd and NMM cmd #225

Closed taldcroft closed 2 years ago

taldcroft commented 2 years ago

Description

As noted in originally in slack there are cases where obsids in nominal schedules had two star catalogs / observations. E.g.

observations = Table(commands.get_observations(obsid=57244))

observations[['obs_start', 'obs_stop']]

<Table length=2>
      obs_start              obs_stop      
        str21                 str21        
--------------------- ---------------------
2009:068:08:40:02.068 2009:068:08:40:02.068
2009:068:08:54:32.010 2009:068:16:16:30.660

Then obsid=57245 had exactly the same observation info and star catalog. This was due to the obsid and NMM command happening at exactly the same time, but with the obsid before the NMM command. This causes the observation code to assign the second obsid prior (in command order) to the NMM command.

This PR looks through the commands for this situation and swaps the commands.

Interface impacts

None

Testing

Unit tests

Functional tests

With this change I regenerated the cmds2.* files and then did this in IPython with the local files. This shows only the one expected observation.

In [5]: Table(get_observations(obsid=57244, scenario='flight'))
Out[5]: 
<Table length=1>
obsid simpos        obs_stop            manvr_start      ...      starcat_date     starcat_idx  source 
int64 int64          str21                 str21         ...         str21            int32      str8  
----- ------ --------------------- --------------------- ... --------------------- ----------- --------
57244 -99616 2009:068:16:16:30.660 2009:068:08:40:12.319 ... 2009:068:08:40:08.068      139097 MAR0909B
jeanconn commented 2 years ago

I was thinking this would work with more than two commands but not more than two of these types? But haven't checked.

jeanconn commented 2 years ago

Comment moved here from slack: Do we want to have more notes about the rules of commands somewhere? I'm not sure where I'd look. For this PR, it might just be a comment that the latest the obsid command will be (in nominal ops) is at the time of the maneuver command.

javierggt commented 2 years ago

I was thinking this would work with more than two commands but not more than two of these types?

This works only if these two commands are consecutive. There can be multiple appearances of the pair, but if there is something in between them it will not work.

jeanconn commented 2 years ago

Gotcha. With my understanding of commanding then, I think while that is unlikely (more than 2 commands) it seems like the code should be fixed to fine with any number of commands and commands in between, as long as the output has these two commands in the right order to work with the determination of observations.

taldcroft commented 2 years ago

The first version of this actually did something robust to having more commands at one time, namely do a sort by date, scs, tlmsid. This relies on the alphabetic ordering of the relevant commands being (luckily) what we need. I thought this was too hacky though. There are actually cases where there is a SIM command at the same time IIRC. I will look into this a little more.

taldcroft commented 2 years ago

See 116ae13. I used this version to regenerate the commands archive, then compared all observations from the new version to those from the previous version (corresponding to what I put on HEAD). There were no differences.

That leads me to conclude that the previous method was OK for now, but I'm also happy to have code that is more robust to future changes in the way commands are put together.