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

overscan.py parallel overscan sigma clip is really a threshold clip

    XMLWordPrintable

Details

    • Bug
    • Status: Done
    • Resolution: Done
    • None
    • ip_isr
    • None
    • 1
    • Data Release Production
    • No

    Description

      I found when trying to run the parallel overscan in overscan.py, that most parallel overscan values were being masked out. I plotted a histogram of the values, and they look reasonable, and only a few should be masked out. See : Parallel_Overscan_Values_Amp_C10_16Nov22.pdf
      The problem is that at this line:
      https://github.com/lsst/ip_isr/blob/e3fba828a5a18f981ebfd0a6426b864e7888fb3f/python/lsst/ip/isr/overscan.py#L221
      The threshold is treated as a number of sigma, but at this line:
      https://github.com/lsst/ip_isr/blob/e3fba828a5a18f981ebfd0a6426b864e7888fb3f/python/lsst/ip/isr/isrFunctions.py#L126
      the threshold is a scalar value. When I set isrConfig.overscan.numSigmaClip=30.0, then it behaved as it should. I don't know whether the right fix is to change the name of numSigmaClip or to re-write makeThresholdMask.

      Attachments

        Activity

          No builds found.
          cslage Craig Lage created issue -
          czw Christopher Waters made changes -
          Field Original Value New Value
          Attachment dm37022-czw.png [ 64709 ]
          czw Christopher Waters made changes -
          Attachment dm37022-czw-1.png [ 64710 ]
          czw Christopher Waters added a comment - - edited

          I'm changing this to continue to use numSigmaClip and makeThresholdMask as they are, but changing the threshold to use {{numSigmaClip * serialOverscanSigmaResidual.  }}This is slightly more conservative than the value of 30 (~21-24 ADU at 3-sigma), but fixes the code to be consistent with what the config options say.

          My residual images with this, setting doParallelOverscan=True and confirming that it's running overscan.fitType=MEDIAN_PER_ROW (as DM-34989 hasn't hit the weekly yet) largely fixes the bar in C10.  For my comparison to the plot on slack, I did need to disable doBias.  Bias subtraction runs after the overscan subtraction, so applying the existing bias for this data imprints a negative bias "wave".

          czw Christopher Waters added a comment - - edited I'm changing this to continue to use numSigmaClip  and makeThresholdMask as they are, but changing the threshold to use {{numSigmaClip * serialOverscanSigmaResidual.  }}This is slightly more conservative than the value of 30 (~21-24 ADU at 3-sigma), but fixes the code to be consistent with what the config options say. My residual images with this, setting doParallelOverscan=True  and confirming that it's running overscan.fitType=MEDIAN_PER_ROW  (as DM-34989 hasn't hit the weekly yet) largely fixes the bar in C10.  For my comparison to the plot on slack, I did need to disable doBias .  Bias subtraction runs after the overscan subtraction, so applying the existing bias for this data imprints a negative bias "wave".
          czw Christopher Waters made changes -
          Attachment dm37022-czw.png [ 64709 ]
          czw Christopher Waters made changes -
          Status To Do [ 10001 ] In Progress [ 3 ]
          czw Christopher Waters made changes -
          Epic Link DM-32163 [ 779863 ]
          Team Data Release Production [ 10301 ]
          czw Christopher Waters made changes -
          Reviewers Craig Lage [ cslage ] Eli Rykoff [ erykoff ]
          Status In Progress [ 3 ] In Review [ 10004 ]
          erykoff Eli Rykoff added a comment -

          I worry a lot about the lack of tests ...

          erykoff Eli Rykoff added a comment - I worry a lot about the lack of tests ...
          erykoff Eli Rykoff made changes -
          Status In Review [ 10004 ] Reviewed [ 10101 ]

          Agree on the tests, but this is a general issue, so I've made DM-37039 to do another pass through ip_isr's tests to improve coverage and results.

          czw Christopher Waters added a comment - Agree on the tests, but this is a general issue, so I've made  DM-37039 to do another pass through ip_isr 's tests to improve coverage and results.
          czw Christopher Waters made changes -
          Story Points 1
          czw Christopher Waters made changes -
          Resolution Done [ 10000 ]
          Status Reviewed [ 10101 ] Done [ 10002 ]

          People

            czw Christopher Waters
            cslage Craig Lage
            Eli Rykoff
            Christopher Waters, Craig Lage, Eli Rykoff
            Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

              Created:
              Updated:
              Resolved:

              Jenkins

                No builds found.