-
Notifications
You must be signed in to change notification settings - Fork 3
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
Add starcat_date to observation parameters + fix caching and minor issues #221
Conversation
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. |
This comment was marked as off-topic.
This comment was marked as off-topic.
@javierggt - for testing you need to copy the new 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 |
I started thinking how to make functional tests. For that purpose I created a branch of agasc that includes a function Using the 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:
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. |
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 |
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:
So the 836 you found seems in the right ballpark. I suspect my grep includes obsids that were never run, hence the overcount. |
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 ok with these changes, which now include PR #222 and will include PRs #225 and PR #226. I assume this PR will wait until those are merged. Perhaps the title and description should be changed to include them, or are we certain that those PR merges will have the commit message that reads "Merge pull request..."? There is no such commit after 222 was merged.
The only thing I thought when looking at the code was that the cache is going to be cleared whenever one runs tests, but I guess there is no trivial way to fix that.
I ran the unit tests and some functional tests by reproducing the process that will be followed in the agasc supplement update.
@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. |
Regarding the 836 without star catalogs, they look to all have npnt_enab == False, which at least seems consistent. Regarding
Those were actually planned gyro holds (I'm forgetting why but from the load review notes)
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). |
The ``unique`` parameter can be set to ``True`` to only return unique star | ||
catalog entries. There are numerous instances of a single commanded star | ||
catalogs that is associated with two ObsIDs, for instance ACIS undercover | ||
observations. To get only the first one, set ``unique=True``. |
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.
For the unique flag, the first undercover I tried did not line up with expectations:
In [29]: get_starcats_as_table(start='2020:366:17:04:20.000', stop='2020:366:23:00:00.000')
Out[29]:
<Table length=22>
obsid starcat_date slot idx id type sz mag maxmag yang zang dim res halfw
int64 str21 int64 int64 int64 str3 str3 float64 float64 float64 float64 int64 int64 int64
----- --------------------- ----- ----- --------- ---- ---- ----------------- --------- ------------------- ------------------- ----- ----- -----
24882 2020:366:17:04:20.000 0 1 1 FID 8x8 7.0 8.0 -1174.9607093061636 -468.1481719926585 1 1 25
24882 2020:366:17:04:20.000 1 2 3 FID 8x8 7.0 8.0 -1177.6232024219992 561.4018012757298 1 1 25
24882 2020:366:17:04:20.000 2 3 4 FID 8x8 7.0 8.0 1223.4393109616512 563.8018390366224 1 1 25
24882 2020:366:17:04:20.000 3 4 552075520 BOT 6x6 8.885130882263184 10.390625 -912.6106246537174 -703.8856678520224 8 1 60
24882 2020:366:17:04:20.000 4 5 552086952 BOT 6x6 8.897590637207031 10.40625 -1510.7731283644346 -2392.854408253782 8 1 60
24882 2020:366:17:04:20.000 5 6 552206400 BOT 6x6 8.648664474487305 10.15625 -84.90442138289295 -2367.635642290802 12 1 80
24882 2020:366:17:04:20.000 6 7 552206936 BOT 6x6 6.875301837921143 8.375 2104.7955129097486 1092.1642810692815 16 1 100
24882 2020:366:17:04:20.000 7 8 552213664 BOT 6x6 8.73717975616455 10.234375 1073.65803292352 -944.4668937469213 12 1 80
24882 2020:366:17:04:20.000 0 9 552217088 ACQ 6x6 8.083139419555664 9.59375 168.79558020035054 -2223.1544736547244 12 1 80
24882 2020:366:17:04:20.000 1 10 552081976 ACQ 6x6 9.25229549407959 10.75 -1094.5981762913334 1597.2393211928293 8 1 60
24882 2020:366:17:04:20.000 2 11 552084112 ACQ 6x6 9.63385009765625 11.140625 -782.1356873895911 -292.7544209467494 8 1 60
24882 2020:366:17:04:20.000 0 1 1 FID 8x8 7.0 8.0 -1174.9607093061636 -468.1481719926585 1 1 25
24882 2020:366:17:04:20.000 1 2 3 FID 8x8 7.0 8.0 -1177.6232024219992 561.4018012757298 1 1 25
24882 2020:366:17:04:20.000 2 3 4 FID 8x8 7.0 8.0 1223.4393109616512 563.8018390366224 1 1 25
24882 2020:366:17:04:20.000 3 4 552075520 BOT 6x6 8.885130882263184 10.390625 -912.6106246537174 -703.8856678520224 8 1 60
24882 2020:366:17:04:20.000 4 5 552086952 BOT 6x6 8.897590637207031 10.40625 -1510.7731283644346 -2392.854408253782 8 1 60
24882 2020:366:17:04:20.000 5 6 552206400 BOT 6x6 8.648664474487305 10.15625 -84.90442138289295 -2367.635642290802 12 1 80
24882 2020:366:17:04:20.000 6 7 552206936 BOT 6x6 6.875301837921143 8.375 2104.7955129097486 1092.1642810692815 16 1 100
24882 2020:366:17:04:20.000 7 8 552213664 BOT 6x6 8.73717975616455 10.234375 1073.65803292352 -944.4668937469213 12 1 80
24882 2020:366:17:04:20.000 0 9 552217088 ACQ 6x6 8.083139419555664 9.59375 168.79558020035054 -2223.1544736547244 12 1 80
24882 2020:366:17:04:20.000 1 10 552081976 ACQ 6x6 9.25229549407959 10.75 -1094.5981762913334 1597.2393211928293 8 1 60
24882 2020:366:17:04:20.000 2 11 552084112 ACQ 6x6 9.63385009765625 11.140625 -782.1356873895911 -292.7544209467494 8 1 60
In [30]: get_starcats_as_table(start='2020:366:17:04:20.000', stop='2020:366:23:00:00.000', unique=True)
Out[30]:
<Table length=11>
obsid starcat_date slot idx id type sz mag maxmag yang zang dim res halfw
int64 str21 int64 int64 int64 str3 str3 float64 float64 float64 float64 int64 int64 int64
----- --------------------- ----- ----- --------- ---- ---- ----------------- --------- ------------------- ------------------- ----- ----- -----
24882 2020:366:17:04:20.000 0 1 1 FID 8x8 7.0 8.0 -1174.9607093061636 -468.1481719926585 1 1 25
24882 2020:366:17:04:20.000 1 2 3 FID 8x8 7.0 8.0 -1177.6232024219992 561.4018012757298 1 1 25
24882 2020:366:17:04:20.000 2 3 4 FID 8x8 7.0 8.0 1223.4393109616512 563.8018390366224 1 1 25
24882 2020:366:17:04:20.000 3 4 552075520 BOT 6x6 8.885130882263184 10.390625 -912.6106246537174 -703.8856678520224 8 1 60
24882 2020:366:17:04:20.000 4 5 552086952 BOT 6x6 8.897590637207031 10.40625 -1510.7731283644346 -2392.854408253782 8 1 60
24882 2020:366:17:04:20.000 5 6 552206400 BOT 6x6 8.648664474487305 10.15625 -84.90442138289295 -2367.635642290802 12 1 80
24882 2020:366:17:04:20.000 6 7 552206936 BOT 6x6 6.875301837921143 8.375 2104.7955129097486 1092.1642810692815 16 1 100
24882 2020:366:17:04:20.000 7 8 552213664 BOT 6x6 8.73717975616455 10.234375 1073.65803292352 -944.4668937469213 12 1 80
24882 2020:366:17:04:20.000 0 9 552217088 ACQ 6x6 8.083139419555664 9.59375 168.79558020035054 -2223.1544736547244 12 1 80
24882 2020:366:17:04:20.000 1 10 552081976 ACQ 6x6 9.25229549407959 10.75 -1094.5981762913334 1597.2393211928293 8 1 60
24882 2020:366:17:04:20.000 2 11 552084112 ACQ 6x6 9.63385009765625 11.140625 -782.1356873895911 -292.7544209467494 8 1 60
Yes, unique reduced this to one set of entries. I just thought the second set of entries would have obsid 62647 in the table.
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.
This looks like something else that is behaving a little bit oddly with caching (and now my cache has a different state / obsid.
In [10]: with conf.set_temp('cache_starcats', True):
...: print(get_starcats_as_table(start='2020:366:17:04:20.000', stop='2020:366:23:00:00.000'))
...:
obsid starcat_date slot idx id type sz mag maxmag yang zang dim res halfw
----- --------------------- ---- --- --------- ---- --- ----------------- --------- ------------------- ------------------- --- --- -----
62647 2020:366:17:04:20.000 0 1 1 FID 8x8 7.0 8.0 -1174.9607093061636 -468.1481719926585 1 1 25
62647 2020:366:17:04:20.000 1 2 3 FID 8x8 7.0 8.0 -1177.6232024219992 561.4018012757298 1 1 25
62647 2020:366:17:04:20.000 2 3 4 FID 8x8 7.0 8.0 1223.4393109616512 563.8018390366224 1 1 25
62647 2020:366:17:04:20.000 3 4 552075520 BOT 6x6 8.885130882263184 10.390625 -912.6106246537174 -703.8856678520224 8 1 60
62647 2020:366:17:04:20.000 4 5 552086952 BOT 6x6 8.897590637207031 10.40625 -1510.7731283644346 -2392.854408253782 8 1 60
62647 2020:366:17:04:20.000 5 6 552206400 BOT 6x6 8.648664474487305 10.15625 -84.90442138289295 -2367.635642290802 12 1 80
62647 2020:366:17:04:20.000 6 7 552206936 BOT 6x6 6.875301837921143 8.375 2104.7955129097486 1092.1642810692815 16 1 100
62647 2020:366:17:04:20.000 7 8 552213664 BOT 6x6 8.73717975616455 10.234375 1073.65803292352 -944.4668937469213 12 1 80
62647 2020:366:17:04:20.000 0 9 552217088 ACQ 6x6 8.083139419555664 9.59375 168.79558020035054 -2223.1544736547244 12 1 80
62647 2020:366:17:04:20.000 1 10 552081976 ACQ 6x6 9.25229549407959 10.75 -1094.5981762913334 1597.2393211928293 8 1 60
62647 2020:366:17:04:20.000 2 11 552084112 ACQ 6x6 9.63385009765625 11.140625 -782.1356873895911 -292.7544209467494 8 1 60
62647 2020:366:17:04:20.000 0 1 1 FID 8x8 7.0 8.0 -1174.9607093061636 -468.1481719926585 1 1 25
62647 2020:366:17:04:20.000 1 2 3 FID 8x8 7.0 8.0 -1177.6232024219992 561.4018012757298 1 1 25
62647 2020:366:17:04:20.000 2 3 4 FID 8x8 7.0 8.0 1223.4393109616512 563.8018390366224 1 1 25
62647 2020:366:17:04:20.000 3 4 552075520 BOT 6x6 8.885130882263184 10.390625 -912.6106246537174 -703.8856678520224 8 1 60
62647 2020:366:17:04:20.000 4 5 552086952 BOT 6x6 8.897590637207031 10.40625 -1510.7731283644346 -2392.854408253782 8 1 60
62647 2020:366:17:04:20.000 5 6 552206400 BOT 6x6 8.648664474487305 10.15625 -84.90442138289295 -2367.635642290802 12 1 80
62647 2020:366:17:04:20.000 6 7 552206936 BOT 6x6 6.875301837921143 8.375 2104.7955129097486 1092.1642810692815 16 1 100
62647 2020:366:17:04:20.000 7 8 552213664 BOT 6x6 8.73717975616455 10.234375 1073.65803292352 -944.4668937469213 12 1 80
62647 2020:366:17:04:20.000 0 9 552217088 ACQ 6x6 8.083139419555664 9.59375 168.79558020035054 -2223.1544736547244 12 1 80
62647 2020:366:17:04:20.000 1 10 552081976 ACQ 6x6 9.25229549407959 10.75 -1094.5981762913334 1597.2393211928293 8 1 60
62647 2020:366:17:04:20.000 2 11 552084112 ACQ 6x6 9.63385009765625 11.140625 -782.1356873895911 -292.7544209467494 8 1 60
In [11]: with conf.set_temp('cache_starcats', False):
...: print(get_starcats_as_table(start='2020:366:17:04:20.000', stop='2020:366:23:00:00.000'))
...:
obsid starcat_date slot idx id type sz mag maxmag yang zang dim res halfw
----- --------------------- ---- --- --------- ---- --- ----------------- --------- ------------------- ------------------- --- --- -----
24882 2020:366:17:04:20.000 0 1 1 FID 8x8 7.0 8.0 -1174.9607093061636 -468.1481719926585 1 1 25
24882 2020:366:17:04:20.000 1 2 3 FID 8x8 7.0 8.0 -1177.6232024219992 561.4018012757298 1 1 25
24882 2020:366:17:04:20.000 2 3 4 FID 8x8 7.0 8.0 1223.4393109616512 563.8018390366224 1 1 25
24882 2020:366:17:04:20.000 3 4 552075520 BOT 6x6 8.885130882263184 10.390625 -912.6106246537174 -703.8856678520224 8 1 60
24882 2020:366:17:04:20.000 4 5 552086952 BOT 6x6 8.897590637207031 10.40625 -1510.7731283644346 -2392.854408253782 8 1 60
24882 2020:366:17:04:20.000 5 6 552206400 BOT 6x6 8.648664474487305 10.15625 -84.90442138289295 -2367.635642290802 12 1 80
24882 2020:366:17:04:20.000 6 7 552206936 BOT 6x6 6.875301837921143 8.375 2104.7955129097486 1092.1642810692815 16 1 100
24882 2020:366:17:04:20.000 7 8 552213664 BOT 6x6 8.73717975616455 10.234375 1073.65803292352 -944.4668937469213 12 1 80
24882 2020:366:17:04:20.000 0 9 552217088 ACQ 6x6 8.083139419555664 9.59375 168.79558020035054 -2223.1544736547244 12 1 80
24882 2020:366:17:04:20.000 1 10 552081976 ACQ 6x6 9.25229549407959 10.75 -1094.5981762913334 1597.2393211928293 8 1 60
24882 2020:366:17:04:20.000 2 11 552084112 ACQ 6x6 9.63385009765625 11.140625 -782.1356873895911 -292.7544209467494 8 1 60
24882 2020:366:17:04:20.000 0 1 1 FID 8x8 7.0 8.0 -1174.9607093061636 -468.1481719926585 1 1 25
24882 2020:366:17:04:20.000 1 2 3 FID 8x8 7.0 8.0 -1177.6232024219992 561.4018012757298 1 1 25
24882 2020:366:17:04:20.000 2 3 4 FID 8x8 7.0 8.0 1223.4393109616512 563.8018390366224 1 1 25
24882 2020:366:17:04:20.000 3 4 552075520 BOT 6x6 8.885130882263184 10.390625 -912.6106246537174 -703.8856678520224 8 1 60
24882 2020:366:17:04:20.000 4 5 552086952 BOT 6x6 8.897590637207031 10.40625 -1510.7731283644346 -2392.854408253782 8 1 60
24882 2020:366:17:04:20.000 5 6 552206400 BOT 6x6 8.648664474487305 10.15625 -84.90442138289295 -2367.635642290802 12 1 80
24882 2020:366:17:04:20.000 6 7 552206936 BOT 6x6 6.875301837921143 8.375 2104.7955129097486 1092.1642810692815 16 1 100
24882 2020:366:17:04:20.000 7 8 552213664 BOT 6x6 8.73717975616455 10.234375 1073.65803292352 -944.4668937469213 12 1 80
24882 2020:366:17:04:20.000 0 9 552217088 ACQ 6x6 8.083139419555664 9.59375 168.79558020035054 -2223.1544736547244 12 1 80
24882 2020:366:17:04:20.000 1 10 552081976 ACQ 6x6 9.25229549407959 10.75 -1094.5981762913334 1597.2393211928293 8 1 60
24882 2020:366:17:04:20.000 2 11 552084112 ACQ 6x6 9.63385009765625 11.140625 -782.1356873895911 -292.7544209467494 8 1 60
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.
So if the starcats are just in the cache with one key, I would assume that get_starcats or get_starcats_as_table would need to hold on to the obsid to assign it in a way that makes sense for the undercover in this special case. This is not new for this PR but I think is something I missed in the PR #226 review.
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.
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.
Thanks! I wasn't sure that we actually needed to put the catalog in the cache twice for these observations that share catalog? But using a unique keys does seem simpler than pulling out the one star catalog twice and labeling it again with obs['obsid']
as part of the de-caching in get_starcats.
c4775e8
to
7b03162
Compare
I rebased to fix the merge conflict. This was just silly, I had accidentally put an empty line in |
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.
Looks good to me. I think we might want a unit test of "unique" in get_starcats_as_table, but given the documented (and not documented) functional testing I think this is fine to go without it.
Thanks again for the reviews! |
Description
Add
starcat_date
to the parameters stored in observations.Other things along the way:
clear_caches
was missing the OBSERVATIONS cache, so that was fixed.get_observations
was 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_observations
orget_starcats
test using theflight
scenario so they will run on GRETA.Also includes #225 and #226.
Fixes #220
Interface impacts
starcat_date
to the dict returned byget_observations
date
from being theobs_start
(start of NPNT) tostarcat_date
(commanded star catalog time).cmds2.pkl
andcmds2.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
unique
option 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.