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-28245: Update ConsolidateVisitSummaryTask to use calexp summaryStats. #493

Merged
merged 1 commit into from Apr 5, 2021

Conversation

erykoff
Copy link
Contributor

@erykoff erykoff commented Apr 2, 2021

No description provided.

Copy link
Contributor

@MorganSchmitz MorganSchmitz left a comment

Choose a reason for hiding this comment

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

LGTM! My only comment is not about the change itself, just nitpicking on a docstring clarification you made along the way.

It looks like we're adding a few new fields (zenithDistance etc) we got from the new summaryStatistics. Maybe it's worth mentioning this in the body of the commit message, just so we can easily pinpoint when that was done?

@@ -1081,12 +1080,12 @@ def runQuantum(self, butlerQC, inputRefs, outputRefs):
butlerQC.put(expCatalog, outputRefs.visitSummary)

def _combineExposureMetadata(self, visit, dataRefs, isGen3=True):
"""Make a combined exposure catalog from a list of dataRefs.
"""Make a combined exposure catalog from a list of calexp dataRefs.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think I remember something about trying to avoid using the word "calexp" in user-facing places like docstrings, or did I make that up?

This commit adds additional fields available in the summaryStats,
including detector center ra/decl; zenith distance; zero-point;
and sky level and noise.
@erykoff erykoff merged commit 9ea87ca into master Apr 5, 2021
@erykoff erykoff deleted the tickets/DM-28245 branch April 5, 2021 20: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