Your points about the unit tests are well taken. I think I have fully addressed them as follows:
- I added tests on small images with known expected results.
- I moved BoxGrid (renamed from BoxArr to afw.geom.testUtils and makeRampImage to afw.image.testUtils and added unit tests for them. I also enhanced BoxGrid to work with floating point boxes (Box2D.
Regarding the linearization functors: I plan to keep the current API for the following reasons:
- I think it more natural to persist and unpersist one detector's worth of linearization information at a time, since that matches the granularity of the ISR code. One indication of this is that the argument list is simpler with my API: linearize(fullImage, detector) instead of linearize(ampImage, detector, ampIndex). Admittedly the detector is is passed instead of the amp info record for sanity checking and not due to any fundamental need, but I think the sanity checking is useful.
- By the time we get to linearization we have already assembled the image and all operations are being performed on the entire detector. Thus the calls around there all look the same (pass in the full image).
- It keeps the ISR code simpler. We tend to need to reimplement ISR separately for each camera, so I think this results in a net simplification of code (or will when we have more cameras that implement linearization).
- Note that the existing Subaru code linearized the CCD image as a unit, so this is not a change for HSC and SuprimeCam.
Regarding out of range pixels: you raise a good point. However, pixels that are too large for the table will already be flagged as SAT or SUSPECT unless the table has a range that that is crazily too small. Pixels that are too small can safely be left alone, and we have no reasonable flag for them. It is conceivable that too-large pixels should trigger an exception (and that would be a trivial change); I think that is not needed for too-small pixels.
Regarding obs_subaru: I split the commit into two pieces: first standardize suspect masking, then linearize using the new code. I think it is a small a set of changes as makes sense. I also modified the log calls as you requested and fixed some pyflakes warnings and errors (in a few additional commits) while I was at it.
Please have another look.
Added two linearizers to ip_isr: one for a squared model (obs_subaru) and one for lookup tables (probably DECam and LSST). Linearizers are treated as calibration data products, but they are functors. The squared linearizer gets its coefficient from the amp info, so there is only one and the Subaru camera mapper provides it (the same one regardless of data ID). The table-based linearizer stores the table and a few other coefficients, so users will persist (pickle) one per CCD in the data repo, and this will require an update to the camera mapper paf file.
I updated the subaru-specific ISR task and mapper to use the squared linearizer. It would be useful to run ci_hsc on master and this branch to confirm that nothing changes. Unfortunately I can't get it to run on El Capitan. Of course the unit tests pass (and a Jenkins run passed).