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

Tickets/dm 12480 #6

Merged
merged 2 commits into from Nov 16, 2017
Merged

Tickets/dm 12480 #6

merged 2 commits into from Nov 16, 2017

Conversation

mfisherlevine
Copy link
Contributor

No description provided.

Previous version replicated a lot of things defined in obs_base.
The ticket removes those redundant redefinitions, and cleans up
the unnecessary paths of the path templates. Also changes some
of the persisted dataset types from the deprecated decoratedImage
to be exposures. Also, cleans up some unneeded refCols in the calibs.
obs_comCam only has one raft, so this is never used, so let's
no longer ingest it at all (also, it's always == 'R00' anyway)
Copy link
Contributor

@r-owen r-owen left a comment

Choose a reason for hiding this comment

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

Looks fine. I am wondering if you could simplify the cal entries, but I'm not that familiar with obs_base's settings and getting the entries right is the main thing.

persistable: DecoratedImageF
python: lsst.afw.image.DecoratedImageF
persistable: ExposureF
python: lsst.afw.image.ExposureF
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these entries needed for the cals? I would have expected obs_base to provide suitable defaults.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would expect so too, but couldn't find any. I agree, all these very standard calibration types should be moved to obs_base at some point.

@@ -30,7 +30,6 @@
'date': 'text',
'dateObs': 'text',
'expTime': 'double',
'raft': 'text',
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm glad you could get rid of raft.

@mfisherlevine mfisherlevine merged commit 58be381 into master Nov 16, 2017
@ktlim ktlim deleted the tickets/DM-12480 branch August 25, 2018 06:15
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