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-18531: Add Gen3 implementation to skyCorrection.py. #77

Merged
merged 1 commit into from May 30, 2019

Conversation

czwa
Copy link
Contributor

@czwa czwa commented May 9, 2019

The new Gen3 implementation of the skyCorrection code is serialized,
and duplicates some of the SkyCorrectionTask methods to provide this
serialized form via a run() method (which matches the Gen2
runDataRef() method). Comments have been added to link the Gen2 and
Gen3 methods, and both should be updated for any future code changes.

An additional "rawLinker" input dataset is needed for Gen3, as the
calExp (which has the dataset.visit column populated) cannot find a
link to find the appropriate camera/sky calibration entries. Adding
this rawLinker allows the calExp to connect to the raw data that was
its source, and use that connection to find the calibration files.
This dataset is used only for constructing the quantum graph, and is
removed prior to processing in the task run() method.

@czwa czwa requested a review from natelust May 9, 2019 19:35
Copy link
Collaborator

@natelust natelust left a comment

Choose a reason for hiding this comment

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

I posted to jira about either making common shared code, or splitting up this task into two. I will review further when this is done. If you feel strongly this should be one task, maybe we can sit down and talk through it more.

The comments below referencing "match points" should allow
easier code conversion, as they point out places in the
runDataRef() and run() methods where it should be possible to
compare data.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This comment block should probably not exist. If we were doing other changes to the stack, it would just be for the code to change according to the ticket, it seems that there should be nothing special for the gen3 work, and more over we have not dont it in other places. If you feel it adds valuable information outside the normal to future developers, add it as a code comment, and not as a documentation comment that will be available through api docs, as this is an internal change that only someone coding against this task should know

python/lsst/pipe/drivers/skyCorrection.py Show resolved Hide resolved
def run(self, calExpArray, calBkgArray, skyCalibs, camera):
"""Duplicate runDataRef method for Gen3.

This is written to run in serial for now.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This does not seem appropriate wording for a docstring

@czwa czwa force-pushed the tickets/DM-18531 branch 3 times, most recently from daaf626 to 8f2d3ae Compare May 30, 2019 17:30
The new Gen3 implementation of the skyCorrection code is serialized,
and duplicates some of the SkyCorrectionTask methods to provide this
serialized form via a run() method (which matches the Gen2
runDataRef() method).  Comments have been added to link the Gen2 and
Gen3 methods, and both methods should be updated for any future code
changes.

An additional "rawLinker" input dataset is needed for Gen3, as the
calExp (which has the dataset.visit column populated) cannot find a
link to find the appropriate camera/sky calibration entries.  Adding
this rawLinker allows the calExp to connect to the raw data that was
its source, and use that connection to find the calibration files.
This dataset is used only for constructing the quantum graph, and is
removed prior to processing in the task run() method.
@czwa czwa merged commit 4264176 into master May 30, 2019
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