DM-26545: Add spline linearizer. #159
Conversation
| def search(haystack, needles): | ||
| """Search dictionary 'haystack' for an entry in 'needles' | ||
| """ | ||
| test = [haystack.get(x) for x in needles] | ||
| test = set([x for x in test if x is not None]) | ||
| if len(test) == 0: | ||
| if 'metadata' in haystack: | ||
| return search(haystack['metadata'], needles) | ||
| else: | ||
| return None | ||
| elif len(test) == 1: | ||
| value = list(test)[0] | ||
| if value == '': | ||
| return None | ||
| else: | ||
| return value | ||
| else: | ||
| raise ValueError(f"Too many values found: {len(test)} {test} {needles}") |
There was a problem hiding this comment.
I would likely have had comments here, but we agreed this OOB before hand, so that is fine for now.
There was a problem hiding this comment.
I am open to other more elegant solutions, but I don't know that that can exist until some future point where all of the metadata keywords are standardized completely.
| self._detectorId = search(dictionary, ['DETECTOR', 'detectorId']) | ||
| self._detectorName = search(dictionary, ['DET_NAME', 'DETECTOR_NAME', 'detectorName']) | ||
| self._detectorSerial = search(dictionary, ['DET_SER', 'DETECTOR_SERIAL', 'detectorSerial']) | ||
| self._filter = search(dictionary, ['FILTER', 'filterName']) |
There was a problem hiding this comment.
Is this the raw_md here, i.e. the untranslated header value? Or will this have been through astro_metadata_translator and obs_package translation at this point?
There was a problem hiding this comment.
If the latter then that's fine, if the former then this will cause problems.
There was a problem hiding this comment.
This is an untranslated header value. However, it's the untranslated header value of the calibration product, which is derived either from a user specified value (updateMetadata(filterName='myFavoriteFilter')), or is set by pulling the value from the input exposure, which should have the translated version. My goal of including this was to allow validation (something we don't do yet) to confirm that the calibration matches the input data.
python/lsst/ip/isr/calibType.py
Outdated
| if not self._detectorId and self._detectorSerial: | ||
| self._detectorId = self._detectorSerial |
There was a problem hiding this comment.
If you have no detId but you do have a serial, just make the detId equal to the serial? 🤨 I don't really understand how these can be interchangeable - is there scary downstream logic somewhere/everywhere for dealing with this, or am I misunderstanding?
There was a problem hiding this comment.
I'm removing this, as I think all of the cases that were inconsistent with the detectorId have been purged from the code.
python/lsst/ip/isr/calibType.py
Outdated
| @@ -555,10 +630,8 @@ def fromTable(cls, tableList): | |||
| table = tableList[0] | |||
| metadata = table.meta | |||
| inDict = dict() | |||
|
|
|||
There was a problem hiding this comment.
Some people care deeply about random whitespace additions, but I am not one of those people 🙂
| fitParams = record['FIT_PARAMS'] if 'FIT_PARAMS' in record.columns else np.array([0.0]) | ||
| fitParamsErr = record['FIT_PARAMS_ERR'] if 'FIT_PARAMS_ERR' in record.columns else np.array([0.0]) | ||
| fitChiSq = record['RED_CHI_SQ'] if 'RED_CHI_SQ' in record.columns else np.nan |
There was a problem hiding this comment.
I'm sure there's a good reason for changing all these sentinel values here and above, so that's probably fine, but in a vacuum I certainly prefer None to zero and nan (but like I say, I'm sure there's a good reason, I've just not dug into it here).
There was a problem hiding this comment.
None does not serialize well with astropy tables (where it becomes astropy.io.fits.card.Undefined). I also wanted to keep datatypes consistent for the downstream code.
| tableList.append(catalog) | ||
|
|
||
| if self.tableData: | ||
| catalog = Table([{'LOOKUP_VALUES': value} for value in self.tableData]) | ||
| tableList.append(catalog) | ||
|
|
||
| return(catalog) | ||
| return(tableList) |
There was a problem hiding this comment.
Was the previous returning of catalog just a bug?
| image : `lsst.afw.image.Image` | ||
| Image to be corrected |
There was a problem hiding this comment.
This is obviously waaaay beyond this scope of this review, but does this really only ever run on the Image? Doesn't the variance plane need to be subjected to (some function of) this correction too? Or is that so n-th order as to be deep in the weeds?
There was a problem hiding this comment.
That's the way it has been written for a long time. However, it runs in ip_isr after the variance plane has been constructed, which makes me wonder if it should be being run prior to that step. I think in general this is a small enough term to ignore that issue. Since the nonlinearity is at the bright end, I think the astronomical effects of this would be making significant sources more significant, and possibly biasing surface brightness fits to galaxy cores. I'll think about it more, as it's certainly out of scope here.
| ``"log"`` | ||
| Logger to handle messages (`lsst.log.Log`). |
There was a problem hiding this comment.
I'm actually not familiar with documenting kwargs - does this mean it's considered a mandatory kwarg? Either way, it's not made use of here.
There was a problem hiding this comment.
The lookup table linearizer has log output, and with the goal of "the linearizers should quack identically", I've taken that to mean that they should accept a log kwarg, even if it's unused.
python/lsst/ip/isr/linearize.py
Outdated
| output : `bool` | ||
| If true, a correction was applied successfully. |
There was a problem hiding this comment.
You say you return a single bool, but actually return True, 0.
There was a problem hiding this comment.
I wonder how many reviews have missed that. Fixed the docstrings to match reality.
| delta = interp.interpolate(ampArr.flatten()) | ||
| ampArr -= np.array(delta).reshape(ampArr.shape) | ||
|
|
||
| return True, 0 |
There was a problem hiding this comment.
The fact that you return something saying whether it was applied successfully implies that you might return something else, so I expected to see, from the docs/form of API, to maybe see a copy, a try, and a finally resetting to the copy on fail, and a return False. That seems expensive and probably unnecessary, but just saying what I'd expect to happen given that it says you get a return value if successful here.
There was a problem hiding this comment.
The numeric linearizers (spline/squared/polynomial) have no way to fail. The lookup table can fail, if all pixel values fall outside the lookup table range. This might be an issue with splines, although the low-end of the fit is padded to ensure a linear extrapolation in that direction.
czwa
force-pushed
the
tickets/DM-26545
branch
from
0f55fbf
to
be6806e
Compare
This also reorders how the metadata is added, so a CALIB_ID can be defined by parameters set on the same pass.
czwa
force-pushed
the
tickets/DM-26545
branch
from
be6806e
to
943c655
Compare
This changeset adds the spline linearizer, and some additional metadata handling tools to try and enforce more uniform calibration headers.