Uploaded image for project: 'Data Management'
  1. Data Management
  2. DM-5462

Add non-linearity correction to ISR task

    Details

    • Story Points:
      6
    • Epic Link:
    • Sprint:
      Alert Production X16 - 5, Alert Production F16 - 6
    • Team:
      Alert Production

      Description

      Implement RFC-164

      At the moment some preliminary code is on ticket branches, but this need to be redone once the RFC is finished.

        Attachments

          Issue Links

            Activity

            Hide
            rowen Russell Owen added a comment -

            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.

            Show
            rowen Russell Owen added a comment - 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.
            Hide
            rowen Russell Owen added a comment -

            I realized I had forgotten to update the camera generating code for those obs packages that use such. Fixed. I cleaned up some code in obs_decam at the same time (as a separate commit).

            Show
            rowen Russell Owen added a comment - I realized I had forgotten to update the camera generating code for those obs packages that use such. Fixed. I cleaned up some code in obs_decam at the same time (as a separate commit).
            Hide
            price Paul Price added a comment -

            I disagree about the API, but I don't think it's so important. Thank you for thinking about it and considering my ideas.

            I added some comments to the afw github PR about the test functions; nothing major.

            Thank you for all the clean ups!

            Show
            price Paul Price added a comment - I disagree about the API, but I don't think it's so important. Thank you for thinking about it and considering my ideas. I added some comments to the afw github PR about the test functions; nothing major. Thank you for all the clean ups!
            Hide
            rowen Russell Owen added a comment -

            Thank you for the helpful cleanup suggestions.

            Show
            rowen Russell Owen added a comment - Thank you for the helpful cleanup suggestions.
            Hide
            tjenness Tim Jenness added a comment -

            Russell Owen do you want to add entries for the two new test routines into the developer guide now that we have entries for those types of routines in that document.

            Show
            tjenness Tim Jenness added a comment - Russell Owen do you want to add entries for the two new test routines into the developer guide now that we have entries for those types of routines in that document.

              People

              • Assignee:
                rowen Russell Owen
                Reporter:
                rowen Russell Owen
                Reviewers:
                Paul Price
                Watchers:
                Colin Slater, Hsin-Fang Chiang, Paul Price, Russell Owen, Simon Krughoff, Tim Jenness
              • Votes:
                0 Vote for this issue
                Watchers:
                6 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:

                  Summary Panel