Closed taldcroft closed 2 years ago
taldcroft
commented
2 years ago Although the updated data files are compatible with the code, some unit tests in the release will fail with the new files. For testing I put updated files in ~aldcroft/tmp/kadi/ on HEAD.
javierggt
commented
2 years ago Also, it is not clear to me how this is supposed to replace the use of timelines in the agasc supplement (this might not be related to this P, so perhaps we discuss it elsewhere). In the agasc supplement one needs a table of star-observations, which in an ideal world be something like:
observations = Table(commands.get_observations())
catalogs = commands.get_starcats()
join(observations, starcats, keys=['obsid'])That does not work exactly, because this is a list of ACATable. I suppose I can iterate over these, creating a table, adding obsid column, vstack and then join. Still, just pointing out that I'd rather have a single table.
taldcroft
commented
2 years ago @javierggt - for testing you need to copy the new cmds2.* files I referenced above into a local directory and then point kadi to them via export KADI=<local-kadi-dir>. If you are using events then you need to link/copy events3.db3 as well.
You might have noticed that it seems to work for recent obsids, which is because those are being created in memory on-the-fly and thus had the starcat_date key included.
javierggt
commented
2 years ago I started thinking how to make functional tests. For that purpose I created a branch of agasc that includes a function get_star_observations in agasc.supplement.magnitudes.star_obs_catalogs. See here.
Using the commands-v2 branch of agasc and the branch in this PR, I did the following:
from agasc.supplement.magnitudes import star_obs_catalogs
star_obs_catalogs.load()
stars_obs_v1 = star_obs_catalogs.STARS_OBS
stars_obs_v2 = star_obs_catalogs.get_star_observations()These star-observations should be equivalent or close enough... A simple comparison of OBSIDs and agasc IDs in both:
len(stars_obs_v1): 207219
len(stars_obs_v2): 376708
len(obsid_v1): 32754
len(obsid_v2): 34617
+---------------------|-----------------|---------------------+
| | in obsid_v1 | not in obsid_v1 |
+---------------------|-----------------|---------------------+
| in obsid_v2 | 32584 | 2033 |
| not in obsid_v2 | 170 | |
+---------------------|-----------------|---------------------+
len(agasc_id_v1): 87587
len(agasc_id_v2): 90263
+---------------------|-----------------|---------------------+
| | in agasc_id_v1 | not in agasc_id_v1 |
+---------------------|-----------------|---------------------+
| in agasc_id_v2 | 87561 | 2702 |
| not in agasc_id_v2 | 26 | |
+---------------------|-----------------|---------------------+
I conclude that the code runs and the starcat_date seem to agree. The commands-v2 version seems to produce more star-observations, which is not specific to this PR.
NOTE: these numbers seem not right to me yet. I am taking a closer look and will update once I know better.
javierggt
commented
2 years ago Independently of the numbers above, when I run the following:
from astropy.table import Table
from kadi import commands
commands.conf.commands_version = '2'
observations = Table(commands.get_observations())
np.count_nonzero(observations['starcat_date'].mask)I see 836 observations out of 40232 that have no starcat_date. Would that be expected?
taldcroft
commented
2 years ago I see 836 observations out of 40232 that have no starcat_date. Would that be expected?
This seems reasonable to me. There are normal reasons for this to happen:
It's good to look into these corner cases to find problems. I suspect that with deep digging that there are some oddball cases that are just wrong, but investigating many of them is very time consuming.
One thing is this:
(ska3) ➜ ~ find ~/ska/data/mpcrit1/mplogs/*/*/ofls/ -name 'starcheck.txt' | xargs grep 'No star catalog for obsid' | wc
993 13527 142509So the 836 you found seems in the right ballpark. I suspect my grep includes obsids that were never run, hence the overcount.
taldcroft
commented
2 years ago I just updated the description to note that #225 and #226 are included.
taldcroft
commented
2 years ago @javierggt - I did the comparison of the AGASC star observations to what comes from kadi commands v2 and found not too much actual difference. This comparison uses the MP_STARCAT time as the key. There is a lot of difference in obsids because AGASC uses the originally commanded obsid while kadi uses the as-run obsid, which is different after SCS-107 runs. The takeaway is reinforcing that obsid is not useful in this context since there are different possible definitions.
Also, kadi starts around 2002:001 while AGASC starts in 2003:001. That 2003:001 was arbitrary and conservative, but I think we can push it back to the 2002 date in kadi.
See https://gist.github.com/taldcroft/d870443430466a96a8d5b2cbe5c1faf3. I find just 8 star catalogs that are in kadi v2 and not in AGASC, and zero the other way round.
jeanconn
commented
2 years ago Regarding the 836 without star catalogs, they look to all have npnt_enab == False, which at least seems consistent. Regarding
Segmented maneuvers, which I'm guessing is represented here: https://icxc.harvard.edu/mp/mplogs/2019/DEC0219/oflsa/starcheck.html#obsid47655
Those were actually planned gyro holds (I'm forgetting why but from the load review notes)
· 3 Gyro Holds comprised of 5 attitudes each of which has an earth boresight violation. They start at approximately: o 2019:335:22:00 o 2019:338:13:00 o 2019:341:04:30
I more strict "segmented maneuver" can be seen at something like:
https://icxc.harvard.edu/mp/mplogs/2002/APR0302/oflsc/starcheck.html#obsid2118
And 2118 has what I would expect in the kadi observations (one no-starcat observation, one starcat observation, each with right attitude and timing that seems reasonable).
taldcroft
commented
2 years ago I rebased to fix the merge conflict. This was just silly, I had accidentally put an empty line in states.py and this ended up getting committed.
taldcroft
commented
2 years ago Thanks again for the reviews!
Description
Add
starcat_dateto the parameters stored in observations.Other things along the way:
clear_cacheswas missing the OBSERVATIONS cache, so that was fixed.get_observationswas a reference to the original params dicts in cache, so this allowed for shenanigans if the user played with those outputs. This, of course, came up in testing and took quite a while to figure out.clear_cache()commands in certain tests to ensure independence of results from test order.get_observationsorget_starcatstest using theflightscenario so they will run on GRETA.Also includes #225 and #226.
Fixes #220
Interface impacts
starcat_dateto the dict returned byget_observationsdatefrom being theobs_start(start of NPNT) tostarcat_date(commanded star catalog time).cmds2.pklandcmds2.h5. I've generated these and will place them on HEAD and GRETA. The new data files are expected to be compatible with the release 5.9.2 version in 2022.3 but I'll check that.Running the 5.9.2 unit tests with the new data files gives 3 failures, all expected:
Testing
Unit tests
Independent check of unit tests by Jean
Functional tests
Jean functionally confirmed the
uniqueoption to get_starcats_as_table with this query that gets the two catalogs (same catalog) of an ACIS undercover series.Tom generated new commands archive data files.