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

Bad logic in saturation interpolation config options

    XMLWordPrintable

    Details

    • Type: Bug
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: ip_isr
    • Labels:
      None
    • Story Points:
      1
    • Team:
      Data Release Production

      Description

      Merlin Fisher-Levine was just puzzled as to why it didn't look like the interpolation of saturated pixels had been turned off despite having set doSaturationInterpolation=False. To stop the interpolation from ocurring, he had to set doSaturation=False. The docs say:

      doSaturation = pexConfig.Field(
              dtype=bool,
              doc="Mask saturated pixels?",
              default=True,
          )
       
      doSaturationInterpolation = pexConfig.Field(
              dtype=bool,
              doc="Perform interpolation over pixels masked as saturated?",
              default=True,
          )
      

      indicating some false logic in the task. It looks like the fix should happen with a check for doSaturationInterpolation here: https://github.com/lsst/ip_isr/blob/master/python/lsst/ip/isr/isrTask.py#L1216

      If these configs are meant to operate entirely independently, (a perhaps unlikely, but possible use case: the run() method was passed an image that had the SAT bit set elsewhere), this should be indicated clearly in the docs to avoid potential confusion along the lines "why are/aren't saturated pixels getting interpolated if I set doSaturation=False/doSaturationInterpolation=True).

        Attachments

          Issue Links

            Activity

            Hide
            yusra Yusra AlSayyad added a comment -

            Just to be aware of it. If there's no good place fo a task-level test now, Christopher Waters knows where it should go in DM-15683.

            Show
            yusra Yusra AlSayyad added a comment - Just to be aware of it. If there's no good place fo a task-level test now, Christopher Waters  knows where it should go in  DM-15683 .
            Hide
            czw Christopher Waters added a comment -

            This did seem like there was just a logic issue when this section was refactored.

            On the topic of unit tests, this still would have skipped through the current set of tests in DM-15683, as I'm not testing all option combinations.  I'll try to identify if there are any similar sets of options that should be tested together more thoroughly.

            Show
            czw Christopher Waters added a comment - This did seem like there was just a logic issue when this section was refactored. On the topic of unit tests, this still would have skipped through the current set of tests in DM-15683 , as I'm not testing all option combinations.  I'll try to identify if there are any similar sets of options that should be tested together more thoroughly.
            Hide
            mfisherlevine Merlin Fisher-Levine added a comment -

            I take it that means that this is good to merge without adding tests, as that is being dealt with elsewhere?

            Show
            mfisherlevine Merlin Fisher-Levine added a comment - I take it that means that this is good to merge without adding tests, as that is being dealt with elsewhere?
            Hide
            czw Christopher Waters added a comment -

            Yes, I'll add the tests elsewhere, so you're good to merge.

            Show
            czw Christopher Waters added a comment - Yes, I'll add the tests elsewhere, so you're good to merge.
            Hide
            mfisherlevine Merlin Fisher-Levine added a comment -

            Thanks, done!

            Show
            mfisherlevine Merlin Fisher-Levine added a comment - Thanks, done!

              People

              Assignee:
              mfisherlevine Merlin Fisher-Levine
              Reporter:
              lauren Lauren MacArthur
              Reviewers:
              Christopher Waters
              Watchers:
              Christopher Waters, Jim Bosch, Lauren MacArthur, Merlin Fisher-Levine, Yusra AlSayyad
              Votes:
              0 Vote for this issue
              Watchers:
              5 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved:

                  Jenkins

                  No builds found.