Skip to content
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

DM-23231: Change everywhere to use expId rather than visit+snap #163

Merged
merged 2 commits into from Jan 31, 2020

Conversation

timj
Copy link
Member

@timj timj commented Jan 30, 2020

expId is unique on its own in LSST cameras. visit is now
derived from the groupId and we do not currently have access
to any snap information.

@timj timj requested a review from ktlim January 30, 2020 17:19
@@ -162,7 +162,8 @@ def testDetectorName(self):
name = self.mapper._extractDetectorName({'detector': 29})
self.assertEqual(name, "R10_S02")

name = self.mapper._extractDetectorName({'visit': 3019031900001})
name = self.mapper._extractDetectorName({'obsid': "MC_C_20190319_000001",
'lsstSerial': "ITL-3800C-041"})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe replace one of the above with expId?

config/ingest.py Outdated
}
config.register.unique = ["visit", "detector"]
config.register.visit = ['visit', 'filter', 'dateObs', 'expTime']
config.register.unique = ["obsid", "detector"]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In all of these, this should likely be expId, not obsid, as the former is more likely to be looked up by a user and hence would benefit from the index.

config/ingest.py Outdated
config.register.unique = ["visit", "detector"]
config.register.visit = ['visit', 'filter', 'dateObs', 'expTime']
config.register.unique = ["obsid", "detector"]
config.register.visit = ['visit', 'filter', 'obsid', 'dateObs', 'expTime']
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this table is one-row-per-visit, it should not in general contain obsid or expId, which are only appropriate when visits are always one exposure.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ingest requires that there is a key in unique that is also in visit otherwise it fails. I think I remember now that's why I added obsid to visit.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this means I need to keep visit in unique along with expId.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding visit to unique is better than adding obsid to visit, especially with obsid in unique (which I think will lead to failures). The most frequent lookup should be first in the unique list, so expid, detector, visit is preferable.

export DYLD_LIBRARY_PATH=${LSST_LIBRARY_PATH}

set -e
DATADIR=${OBS_LSST_DIR}/data/input
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could possibly be made into a more generic tool for use by others (including production Butler repos, which would need more safety than this git repo), and I'm not sure examples is the best place for it, but OK.

@timj timj force-pushed the tickets/DM-23231 branch 2 times, most recently from 1a77150 to 004901b Compare January 31, 2020 18:19
expId is unique on its own in LSST cameras. visit is now
derived from the groupId and we do not currently have access
to any snap information.
@timj timj merged commit 16b7b92 into master Jan 31, 2020
@timj timj deleted the tickets/DM-23231 branch January 31, 2020 19:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants