DM-24703: Make linearity a subclass of lsst.ip.isr.IsrCalib #154
Conversation
|
|
||
| import lsst.afw.table as afwTable | ||
| from lsst.daf.base import PropertyList | ||
| from astropy.table import Table |
There was a problem hiding this comment.
Are we moving away from afwTable in favor of astropy.Table?
There was a problem hiding this comment.
Yes for tables that are all in python and not needed to be in C++. Astropy table is faster than afw table for this use case.
There was a problem hiding this comment.
@timj, @ktlim — if that's official Architecture Team policy, can you please make sure it's captured in the Developer Guide? In particular, I think it directly contradicts the statement at https://developer.lsst.io/python/astropy.html that
If the code is already using afw it is strongly preferred that afw equivalents be used until such time as specific afw interfaces are deprecated
There was a problem hiding this comment.
It was more a discovery by @czwa that he learned when he was writing these calibration tasks (which means Defects is slow at the moment and can be improved when it becomes a subclass of the new calibration base class). We haven't formally written that discovery down anywhere.
There was a problem hiding this comment.
That's fine, but I think we do need to make sure that this is captured as part of appropriate policy. “Performance concerns” alone are not an adequate reason to ignore the Dev Guide.
There was a problem hiding this comment.
I very much endorse the idea of making an exception to this rule for afw.table. But I think this is an easy case to make that call on, and writing a policy to cover all cases (including the role of pandas vs. afw.table for Source/Object catalogs) is a lot harder. I hope we can capture that action without blocking this ticket on it.
There was a problem hiding this comment.
I hope we aren't considering blocking this ticket. The performance hit was pretty bad if I recall correctly.
There was a problem hiding this comment.
This ticket doesn't seem to have anything to do with mitigating a performance hit; I don't really understand why that's coming up here.
However, I don't see any need to block this work, as long as the Architecture Team can reassure us that a) it doesn't actually contravene the Developer Guide, and b) they'll be updating the Developer Guide to provide appropriate guidance in future.
There was a problem hiding this comment.
This code is bringing this class into compliance with the pre-existing IsrCalib which is the thing that required astropy tables for performance.
There was a problem hiding this comment.
Ah; it's a shame we didn't take that opportunity to update the Dev Guide; good thing we caught it now! :-)
| Coefficients to use in correction. Indexed by amplifier | ||
| names. The format of the array depends on the type of | ||
| correction to apply. | ||
| linearityType : `dict` [`str`, `str`] |
There was a problem hiding this comment.
Having this as a dictionary would imply that, in principle, each amp would have a different linearity type. But i don't think that's currently the case, correct?
There was a problem hiding this comment.
Each amp can have a different linearity type, and the correction will apply whatever is defined. We currently don't have a way to measure different linearity types simultaneously in cp_pipe, but such a calibration could be constructed if necessary. This is largely a carryover from the cameraGeom defined linearity parameters, which were amplifier based.
| correction to apply. | ||
| linearityType : `dict` [`str`, `str`] | ||
| Type of correction to use, indexed by amplifier names. | ||
| linearityBBox : `dict` [`str`, `lsst.geom.Box2I`] |
There was a problem hiding this comment.
What is the exact meaning of this bounding box? what are the regions where the correction is valid and where it is not? I'm a bit confused by the explanation n the docstring (sorry!)
There was a problem hiding this comment.
Again, carryover from the cameraGeom definitions. The individual amplifiers have been assembled and trimmed so this is used to define the region to apply that amplifier's correction. This means that the correction can be run without passing a detector (which also defines the BBox). The alternative is to make linearizer.applyLinearity() require the detector argument.
Add detectorId as a valid parameter to IsrCalib.
The calibration fields used for equivalence were not set well. Instead of passing text strings around, we should use a camera/detector objects and grab the fields we care about in a single way. These are now parameters of the updateMetadata method, which should now force all calibrations to have consistent field usage. This also defines the CALIB_ID.
This makes the linearity dataset a subclass of IsrCalib, which hopefully gives better code reuse and consistency between datasets.