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-20692: Update existing PipelineTasks to new API #307
Conversation
del staticSkyModelOutputRefs.nImage | ||
|
||
templateCoadd = self.assembleStaticSkyModel.runQuantum(butlerQC, staticSkyModelInputRefs, | ||
staticSkyModelOutputRefs) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A while ago, we talked about moving this step into a sibling PipelineTask
in the same pipeline, so the static sky model coadd would just be an input dataset to CompareWarp
. Is that idea still alive for the future, and we're continuing to do things this way to maintain similarity with Gen2 a while longer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes! Once we remove Gen2, we won't need to run assembleCoadd as a subtask because we can run assembleCoadd twice in a pipeline.
The first time to generate the template and second time (with compareWarp) which uses that template. That will be a good time to re-work the dataset names too.
This mess will only stay as long as we need to support Gen2 and Gen3
supplementaryData.imageScalerList) | ||
|
||
# In Gen3, we have to check to see if the spanSetMaskList | ||
# derived from the PSFMatchedWarps matches the direct tempExpRefList |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand the comment (in part because I don't see anything that looks like a "spanSetMaskList" in the block below). Are you checking that the sets of data IDs for the direct warps and PSF-matched warps are the same? If that is a test that can fail due to something other than a logic bug in the code, it probably shouldn't be an assertion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
spanSetMaskList
and it is produced in the line above and is an argument to AssembleCoaddTask.run()
below. It's the masks that get applied to the warps in the tempExpRefList
and its crucial that the right mask get applied to the right warp.
I don't actually think an assertion is appropriate here, but it was the safest temporary stopgap. But we should talk about this today. This section is why wanted access to RC2 to test. Will you help me with that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pushed a solution that I'm happy with for as long as Gen2 and Gen3 must live side by side in peace. Could be more clever, but I was aiming for readability.
Confirmed that it works on:
- a RC2 Gen3 repo covering this edge case (thanks for the help getting that setup)
pipetask -b /project/yusra/gen3/RC2/repo-oracle/butler.yaml -i warps -o debug1 -d "tract=9615 and patch=28 and abstract_filter='z'" -p lsst.meas.base -p lsst.ip.isr -p lsst.pipe.tasks run -t assembleCoadd.CompareWarpAssembleCoaddTask:cwact -C cwact:config/compareWarpAssembleCoadd.py
- a gen2 commandline task,
- and a Gen2 coaddDriver running now.
if not warpRef.datasetExists(warpName): | ||
self.log.warn("Could not find %s %s; skipping it", warpName, warpRef.dataId) | ||
return None | ||
warp = warpRef.get(datasetType=warpName, immediate=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that Gen3 only supports immediate
for backwards compatibility with Gen2 (I think - it might be an accident, but I assume @natelust added support intentionally), and the default for Gen2 has been True
for a while, it's better in new code just to leave it out.
bd36738
to
9fbe7f6
Compare
During transition to gen3 middleware some tasks were converted to an in-progress PipelineTask API. This ticket converts those tasks to the API that is expected to be used going forward.
9fbe7f6
to
70d6908
Compare
During transition to gen3 middleware some tasks were converted to in-progress PipelineTask API. This ticket converts those tasks to the API that is expected to be used going forward.