DM-7847: Add mid-level drivers for measurement algorithms #1020
Conversation
enourbakhsh
force-pushed
the
tickets/DM-7847
branch
from
78474d6 to
dd1ce59
Compare
| def setDefaults(self): | ||
| super().setDefaults() | ||
| if self.deblender == "scarlet": | ||
| self.deblend.retarget(scarlet.ScarletDeblendTask) | ||
| elif self.deblender == "meas_deblender": | ||
| self.deblend.retarget(measDeblender.SourceDeblendTask) |
There was a problem hiding this comment.
Would it be better to rename this method to setDeblender() and just call it in the setDefaults() method? This way our __setattr__ method becomes:
def __setattr__(self, key, value):
super().__setattr__(key, value)
if key == "deblender":
self.setDeblender()It just makes more sense to me if I add a setDeblender() method because we might want to put other things in setDefaults() that don’t need to be set every time the deblender is changed.
There was a problem hiding this comment.
After looking at the code I actually think that it would be better to separate this into multiple tasks, a SingleBandMeasurementDriverTask and a MultiBandMeasurementDriverTask that both inherit from a MeasurementDriverBaseTask. I would also break up run into multiple subtasks that are implemented in the MeasurementDriverBaseTask (ie. _detection and measurement) that are the same for both the single-band and multi-band versions.
This would allow you to give more flexibility to the multi-band version, which could optionally use https://github.com/lsst/drp_tasks/blob/main/python/lsst/drp/tasks/assemble_chi2_coadd.py to build a chi^2 coadd and use that for detection (as opposed to having to choose a reference band). So I would make band a config option and if band is None (the default), it would detect on a chi^2 coadd and perform measurement in all bands, producing a measurement catalog in each band. Otherwise if a user wants measurements in multiple bands then they would have to run the driver multiple times, doing detection and deblending again even though the results will be the same.
| deblender = pexConfig.ChoiceField[str]( | ||
| doc="The deblender to use.", | ||
| default="meas_deblender", | ||
| allowed={"meas_deblender": "Deblend using meas_deblender", "scarlet": "Deblend using scarlet"}, | ||
| ) |
There was a problem hiding this comment.
I don't think that this option is necessary. You already set the deblender in deblend, so having a config option that could accidentally be misaligned with the target deblend seems like an unnecessary option that could lead to user error.
This will also allow you to remove the other methods implemented in this class below.
| def setDefaults(self): | ||
| super().setDefaults() | ||
| if self.deblender == "scarlet": | ||
| self.deblend.retarget(scarlet.ScarletDeblendTask) | ||
| elif self.deblender == "meas_deblender": | ||
| self.deblend.retarget(measDeblender.SourceDeblendTask) |
| def run( | ||
| self, | ||
| image, | ||
| bands=None, | ||
| band=None, | ||
| mask=None, | ||
| variance=None, | ||
| psf=None, | ||
| wcs=None, | ||
| photo_calib=None, | ||
| id_generator=None, | ||
| ): |
There was a problem hiding this comment.
I would recommend using type hints in your function signature, then you can remove the types from the docstring of Parameters. This prevents bitrotting and will also help people using an IDE with a mypy-like linter.
| image: `~lsst.afw.image.Exposure` or `~lsst.afw.image.MaskedImage` or | ||
| `~lsst.afw.image.Image` or `np.ndarray` or | ||
| `~lsst.afw.image.MultibandExposure` or | ||
| `list` of `~lsst.afw.image.Exposure` | ||
| The image on which to detect, deblend and measure sources. If | ||
| provided as a multiband exposure, or a list of `Exposure` objects, | ||
| it can be taken advantage of by the 'scarlet' deblender. When using | ||
| a list of `Exposure` objects, the ``bands`` parameter must also be | ||
| provided. | ||
| bands: `str` or `list` of `str`, optional | ||
| The bands of the input image. Required if ``image`` is provided as | ||
| a list of `Exposure` objects. Example: ["g", "r", "i", "z", "y"] | ||
| or "grizy". | ||
| band: `str`, optional | ||
| The target band of the image to use for detection and measurement. | ||
| Required when ``image`` is provided as a `MultibandExposure`, or a | ||
| list of `Exposure` objects. | ||
| mask: `~lsst.afw.image.Mask`, optional | ||
| The mask for the input image. Only used if ``image`` is provided | ||
| as an afw `Image` or a numpy `ndarray`. | ||
| variance: `~lsst.afw.image.Image`, optional | ||
| The variance image for the input image. Only used if ``image`` is | ||
| provided as an afw `Image` or a numpy `ndarray`. | ||
| psf: `~lsst.afw.detection.Psf`, optional | ||
| The PSF model for the input image. Will be ignored if ``image`` is | ||
| provided as an `Exposure`, `MultibandExposure`, or a list of | ||
| `Exposure` objects. | ||
| wcs: `~lsst.afw.image.Wcs`, optional | ||
| The World Coordinate System (WCS) model for the input image. Will | ||
| be ignored if ``image`` is provided as an `Exposure`, | ||
| `MultibandExposure`, or a list of `Exposure` objects. | ||
| photo_calib : `~lsst.afw.image.PhotoCalib`, optional | ||
| Photometric calibration model for the input image. Will be ignored | ||
| if ``image`` is provided as an `Exposure`, `MultibandExposure`, or | ||
| a list of `Exposure` objects. |
There was a problem hiding this comment.
I think that this API is confusing. Instead of having three optional sets of parameters in run, one for Exposures, and one for Images (which requires a lot of additional parameters that are ignored if the image is an Exposure), and one for MultibandExposures, I recommend making run take an Exposure and create a new method, runFromImage that takes the parameters required if an image is passed and creates an exposure from those images to call the normal run. I think that this will make the API easier for users and simplify the codebase. See my other comments about single-band vs multiband
No description provided.