DM-17045: Convert [SafeClip/CompareWarp]AssembleCoadd to PipelineTask #257
Conversation
There was a problem hiding this comment.
Looks good. I have tested dcrAssembleCoaddTask with these changes, and it appears to work as expected. I had mostly only requests for additional docstrings and code comments, which are hopefully easy for you to add.
I have the one-line change to test_dcrAssembleCoadd.py on my machine that does the easy fix to make the tests pass. I can commit and push that if you like.
| inputWarps = pipeBase.InputDatasetField( | ||
| doc=("Input list of warps to be assemebled i.e. stacked." | ||
| "WarpType (e.g. direct, psfMatched) is controlled by we warpType config parameter"), | ||
| # TODO: Change this to user-specified coaddName |
There was a problem hiding this comment.
Does this TODO have a ticket?
| @@ -149,10 +150,50 @@ class AssembleCoaddConfig(CoaddBaseTask.ConfigClass): | |||
| doc=("Attach a piecewise TransmissionCurve for the coadd? " | |||
| "(requires all input Exposures to have TransmissionCurves).") | |||
| ) | |||
| inputWarps = pipeBase.InputDatasetField( | |||
There was a problem hiding this comment.
Not part of this ticket, but could you modify removeMaskPlanes, doMaskBrightObjects, brightObjectMaskName, and doAttachTransmissionCurve above to have the same format of one parameter per line like the rest of the config?
| @@ -309,8 +350,54 @@ def __init__(self, *args, **kwargs): | |||
|
|
|||
| self.warpType = self.config.warpType | |||
|
|
|||
| @classmethod | |||
| def getOutputDatasetTypes(cls, config): | |||
There was a problem hiding this comment.
A one-line docstring would be helpful here.
| return outputTypeDict | ||
|
|
||
| @classmethod | ||
| def getInputDatasetTypes(cls, config): |
There was a problem hiding this comment.
A one-line docstring would be helpful here.
| raise RuntimeError("Gen3 Bright object masks unavailable. Set config doMaskBrightObjects=False") | ||
| return inputTypeDict | ||
|
|
||
| def adaptArgsAndRun(self, inputData, inputDataIds, outputDataIds, butler): |
There was a problem hiding this comment.
A docstring would be especially helpful here. To the uninitiated, it is unclear what "adaptArgs" really means.
There was a problem hiding this comment.
Adding a substantial docstring. adaptArgsAndRun is the runDataRef of Gen3 world. I was hoping this would be called runQuantum, but what's nowrunQuantum turned into a beast that's impossible to override. I'm hoping after January 31st we have a design review and can change some of these names.
|
|
||
| butlerShim = ShimButler(butler) | ||
|
|
||
| skyMap = inputData["skyMap"] |
There was a problem hiding this comment.
Is this (and the following lines) reading in a gen2 butler and modifying it to look like input from a gen3 butler? I realize this code may all disappear once we've transitioned, but in the meantime a short comment about what is being done would be helpful.
There was a problem hiding this comment.
Comment added. Answer: It's the other way around. It's taking a Gen 3 Butler lsst.daf.butler.butler and making it look like a gen 2 butler, with "DataRefs." lsst.daf.butler.ShimButler is a Gen3 construct that quacks like a Gen2 butler. Also, pipelineTask will only call adaptArgsAndRun and with a Gen3 butler. CmdlineTask will only call runDataRef and with a gen 2 butler DataRefs.
| bbox=afwGeom.Box2I(afwGeom.Point2I(0, 0), afwGeom.Extent2I(1, 1)), | ||
| imageOrigin="LOCAL", immediate=True) for tempExpRef in tempExpRefList] | ||
| bbox=afwGeom.Box2I(coaddExposure.getBBox().getMin(), | ||
| afwGeom.Extent2I(1, 1)), immediate=True) |
There was a problem hiding this comment.
Will this still be necessary with the new butler, or is this just for compatibility during the transition?
There was a problem hiding this comment.
ShimButler's _sub bypass implementation currently doesn't take imageOrigin="LOCAL". If you think you'll need that, we could file a ticket and @TallJimbo can add it. For this use case, the workaround didn't seem any more complex.
There was a problem hiding this comment.
DM-17416. I had actually tried to support this, but got the parameter name wrong ("origin" instead of "imageOrigin"). But no objection to the workaround.
As a side note, once we don't need to worry about backwards compatibility, the single-pixel trick should be unnecessary, as in Gen3 you should be able to get all Exposure components directly.
| @@ -1607,6 +1739,13 @@ class CompareWarpAssembleCoaddConfig(AssembleCoaddConfig): | |||
| dtype=float, | |||
| default=0.05 | |||
| ) | |||
| psfMatchedWarps = pipeBase.InputDatasetField( | |||
There was a problem hiding this comment.
psfMatchedWarps doesn't appear to be used anywhere directly. Why is this a different parameter from inputWarps, with checks to make sure the warpType is psfMatched?
There was a problem hiding this comment.
Clarified in the comment. Some more background. Imagine a shared-nothing cluster, where each node does not have access to shared disk. The workflow system decides which input to copy from the datastore to which nodes and then which tasks to run on those nodes. Only data products declared as a InputDatasetField will be on disk. InputDatasetField is what tells "preflight" which data products your task needs. The assemble tasks are cheaty in the sense that we access the data in the run method, but still has to follow the rules and announce what it's going to use.
Separate question of hardcoding which warpType to use for the image-differencing/artifact-finding part of the algorithm. Image-differencing definitely won't work with "direct". And we don't have anything else at the moment. (If we do something fancier with DCR coadd, we'll have to rework this anyway I think). The inputWarps (which actually get stacked in the end) can still be whatever warpType the user wants.
|
|
||
| Alternatively, to use another algorithm with existing warps, retarget the CoaddDriverConfig to | ||
| another algorithm like: | ||
| def _noTemplateMessage(self, warpType): |
There was a problem hiding this comment.
This looks like a helpful error message, thank you for adding it.
python/lsst/pipe/tasks/coaddBase.py
Outdated
|
|
||
|
|
||
| def makeSkyInfo(skyMap, tractId, patchId): | ||
| """! Construct SkyInfo Struct |
There was a problem hiding this comment.
Drop the exclamation point and whitespace.
yalsayyad
force-pushed
the
tickets/DM-17045
branch
4 times, most recently
from
014a4c6
to
a5a60c0
Compare
* Add necessary input/output data configs for PipelineTask
* Add ability to create coadds directly from a list of dataRefs to warps.
This was originally ticketed under DM-13504.
* Remove uninitialized list from the `runDataRef` argument list
yalsayyad
force-pushed
the
tickets/DM-17045
branch
from
a5a60c0
to
9272bac
Compare
This was originally ticketed under DM-13504.
runDataRefargument list