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.
Your points about the unit tests are well taken. I think I have fully addressed them as follows:
Regarding the linearization functors: I plan to keep the current API for the following reasons:
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.