Repository navigation
DM-56114: Add optional lsst.image format for output images - #471
Conversation
| bbox = pipeBase.connectionTypes.Input( | ||
| doc="Bounding box of exposure to determine the geometry of the output template.", | ||
| name="{fakesType}calexp.bbox", | ||
| storageClass="Box2I", |
There was a problem hiding this comment.
I think you need to add a deprecated bbox connection here, because connections generate config fields and we don't want to break our ability to read only slightly old config files.
| unit=astropy.units.nJy, | ||
| instrument=data_id["instrument"], | ||
| visit=data_id["visit"], | ||
| ) |
There was a problem hiding this comment.
I wonder if it would be better to start from the input difference image and then reassign just the image/mask/variance (and maybe not even the last of those?), instead of doing a new from_legacy conversion.
There was a problem hiding this comment.
Good point, that seems safer and more efficient. I need to clean up the background I attached in subtractImages, but I shouldn't have added that in the first place. Background subtraction for the difference image is only actually done in detectAndMeasure.
98c0a40 to
c6f99e3
Compare
f06448c to
48262d4
Compare
TallJimbo
left a comment
TallJimbo
left a comment
There was a problem hiding this comment.
Overall looks good, but I've got two big-picture issues I'd like to see addressed:
-
I think a lot of tests could be replaced by a much smaller number of calls to
lsst.images.tests.compare_visit_image_to_legacy. -
The
exposure_recordthatfrom_legacynow takes is there to make it possible to reconstruct anObservationInfo. I'm sorry I didn't realize the importance of the case where you've already got one of those, but I think it would be a lot better to makeobs_infoan optional argument toVisitImage.from_legacy(andDifferenceImage.from_legacy) that could be provided instead ofexposure_record. That would simplify the code here to the same pattern you already use for other components that are only on the future types and basically just get passed through, while avoiding the chance of any metadata loss.
| for maskPlane in self.config.metricsMaskPlanes: | ||
| try: | ||
| metricsMaskPlanes.append(maskPlane) | ||
| metricsMaskPlanes[maskPlane] = difference.mask.getPlaneBitMask(maskPlane) |
There was a problem hiding this comment.
I think this is a global lookup, too. It's an instance method, but I think under the hood it's calling the same global stuff.
Or at least this usually references the global mask plane dict. There might be some edge cases where it's possible for a Mask to have different one, but usually they don't.
|
I should add that if you want to try to maintain support for taking legacy |
…storage formats This adds a dependency on `lsst.images`
Only relevant with the `lsst.images` conversion, and the original error can be raised if `raiseOnUndefinedMaskMap` is set.
…st.images format images
…mage in future mode
The input image already carries the kernel, template records, observation metadata, PSF, and photometric scaling, so only the pixels, mask, and background need to be written back. The subtractImages conversion no longer attaches the background fit with the kernel, which is unused, so the detectAndMeasure background is the only one recorded.
The score image variant runs its final detection on the score image, so the first-pass detections used to fit the background stayed in the DETECTED planes of the output difference image.
In future mode the background is recorded on the output images, so the separate difference_background dataset is no longer written.
Needs the daf_butler formatter for ConvolutionKernel datasets.
48262d4 to
3f74347
Compare
3f74347 to
83a5f3d
Compare
No description provided.