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-29310: GetMultiTractCoaddTemplateTask for multi-tract image difference task #194

Merged
merged 1 commit into from Sep 1, 2021

Conversation

yalsayyad
Copy link
Contributor

GetMultiTractCoaddTemplateTask warps coadds from multiple tracts onto detector geometry

Copy link
Contributor

@isullivan isullivan left a comment

Choose a reason for hiding this comment

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

Code looks fine, and I like how this can be written using Gen 3. I will write up my broader concern on the Jira ticket.


__all__ = ["GetCoaddAsTemplateTask", "GetCoaddAsTemplateConfig",
"GetCalexpAsTemplateTask", "GetCalexpAsTemplateConfig"]
"GetCalexpAsTemplateTask", "GetCalexpAsTemplateConfig",
"GetMultiTractCoaddTemplateTask", "GetMultiTractCoaddTemplateTask"]
Copy link
Contributor

Choose a reason for hiding this comment

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

"GetMultiTractCoaddTemplateTask" appears twice, no "GetMultiTractCoaddTemplateConfig"

Comment on lines +537 to +539
self.warp.warpingKernelName = 'lanczos5'
self.coaddPsf.warpingKernelName = 'lanczos5'
Copy link
Contributor

Choose a reason for hiding this comment

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

Why lanczos5 and not lanczos3?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

DRP uses lanczos5 for CoaddPsfs during deep coaddition because DM-17429. Option is either to put this config here slowing down AP slightly or put it in the obs_package DPR.yaml. Either is fine with me. Both I and Tanaka-san found that warping with 5 only takes 5% longer: DM-18295

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 it's fine to put this here. If needed in AP, we can downgrade it in the APpipe config.


Parameters
----------
inputs : dict of task Inputs
Copy link
Contributor

Choose a reason for hiding this comment

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

Put dict in backticks


Returns
-------
coaddExposures : list of DeferredDatasetHandles to `lsst.afw.image.Exposure`
Copy link
Contributor

Choose a reason for hiding this comment

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

Please expand this to lsst.daf.butler.DeferredDatasetHandle, and put in backticks.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a Raises section to document the case when pipeBase.NoWorkFound is raised.

result : `struct`
return a pipeBase.Struct:
- ``outputExposure`` : a template coadd exposure assembled out of patches

Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a Raises section to document the pipeBase.NoWorkFound and RuntimeError

Comment on lines +665 to +680
if nPatchesFound == 0:
raise pipeBase.NoWorkFound("No patches found to overlap detector")
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this case be caught by the check in getOverlappingExposures above?

Comment on lines +695 to +710
templateExposure.setFilterLabel(coaddPatch.getFilterLabel())
templateExposure.setPhotoCalib(coaddPatch.getPhotoCalib())
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably unimportant, but note that this code grabs the filter label and photocalib from the last patch that was processed, while the single-tract version of getTemplate gets it from the first.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because "band" is in the dimensions of the 'coaddExposures' inputs, they are guaranteed to be the same. Will add inline comments explaining.

@isullivan
Copy link
Contributor

I forgot to add, please edit the message printed with the RuntimeError on line 177 to point users to the new Task, instead of just printing that multiple tracts are not yet supported.

warped = self.warper.warpExposure(finalWcs, coaddPatch, maxBBox=finalBBox)

# Check if warped image is viable
if warped.getBBox().getArea() == 0:
Copy link
Contributor

Choose a reason for hiding this comment

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

When working with what I believe is very similar code, this check didn't trigger when there wasn't overlap, but adding or not np.any(np.isfinite(warped.getImage().array)) worked to check that it wasn't a viable warped image and should be skipped.

@yalsayyad yalsayyad force-pushed the tickets/DM-29310 branch 2 times, most recently from d581380 to b22ab35 Compare August 30, 2021 02:55
warps coadds from multiple tracts onto detector geometry
@yalsayyad yalsayyad merged commit e5a99e8 into master Sep 1, 2021
@yalsayyad yalsayyad deleted the tickets/DM-29310 branch September 1, 2021 00:52
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

3 participants