DM-22643: convert visualizeVisit to gen3 #366
Conversation
pipelines/VisualizeVisit.yaml
Outdated
| binning: 8 | ||
| connections.inputExp: 'calexp' | ||
| connections.outputExp: 'calexpBin' | ||
| moz: |
There was a problem hiding this comment.
maybe use more descriptive labels for these tasks
| ) | ||
|
|
||
|
|
||
| class VisBinTask(pipeBase.PipelineTask, |
There was a problem hiding this comment.
It is more verbose, but I think our coding standards perfer longer descriptive names, so something like VisualizeBinTask. On that note, I am not sure what the "bin" is in this name, maybe be more descriptive there.
There was a problem hiding this comment.
Makes sense to me. Renamed to Visualize{Action}ExpTask, as they are tasks that Bin/Mosaic Exposure data for Visualization.
|
|
||
| class VisBinTask(pipeBase.PipelineTask, | ||
| pipeBase.CmdLineTask): | ||
| """Task for focal plane visualization. |
There was a problem hiding this comment.
Please expand this doc string just a bit, maybe some what/and or how
pipelines/VisualizeVisit.yaml
Outdated
| config: | ||
| binning: 8 | ||
| connections.inputExp: 'calexp' | ||
| connections.outputExp: 'calexpBin' |
There was a problem hiding this comment.
Do you need to set these since it appears you are using the default names?
There was a problem hiding this comment.
Probably not. It's likely an artifact from me changing those defaults before making the PR.
|
|
||
| binned = inputExp.getMaskedImage() | ||
| binned = afwMath.binImage(binned, self.config.binning) | ||
| outputExp = afwImage.makeExposure(binned) |
There was a problem hiding this comment.
if this is going to be an exposure, you should copy all the associated info from an exposure, ie the exposure info and such.
| if hasattr(image, "getImage"): | ||
| image = image.getImage() | ||
|
|
||
| image = afwMath.rotateImageBy90(image, detector.getOrientation().getNQuarter()) |
There was a problem hiding this comment.
You should check if they actually need rotated or put a comment saying that rotateImageBy90 does the check, It would be a shame to do lots of linear algebra with identity matrices
| pipelineConnections=VisMosaicConnections): | ||
| """Configuration for focal plane visualization. | ||
| """ | ||
| binning = pexConfig.Field( |
There was a problem hiding this comment.
So this is binning done on the focal plane exposure that is done from binned exposures? So if you bin exposures by 8, and then set this binning to 8 the final is 64 times smaller? This seems a bit unclear. If I am correct (or if I am not) please write more doc string to describe this, and maybe put it in comments. Someone just reading the configs might guess that this value must be set to match the other from the other task for some reason.
There was a problem hiding this comment.
It is multiplicative. I'll update the docstring to make that clear, and to mention why this is a good thing.
There was a problem hiding this comment.
I've updated the docstring again to fix my incorrect understanding, after some additional testing from Yusra and Lee. The two numbers must agree between tasks, and so I've added a contract enforcing that to the default pipeline.
czwa
force-pushed
the
tickets/DM-22643
branch
2 times, most recently
from
8c03013
to
28444d3
Compare
| description: Visualize full visit/focal plane with binned and mosaic images. | ||
| tasks: | ||
| binning: | ||
| class: lsst.pipe.tasks.visualizeVisit.VisualizeBinExpTask |
There was a problem hiding this comment.
You should not even need to specify class here, just do binning: lsst.pipe.tasks.visualizeVisit.VisualizeBinExpTask
Add gen3 implementation of visualizeVisit.