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

Many images have few or no good pixels when running ptc.py

    XMLWordPrintable

    Details

    • Type: Improvement
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: cp_pipe, ip_isr, obs_lsst
    • Labels:
      None

      Description

      While debugging DM-27458, I noticed that the reason for many of the failures was that some images had few or no good pixels, and this caused the FFT algorithm that calculates the covariances to fail.  While some amps have some defects, most pixels should be good.  I tracked down one failure to raft R11, sensor S12, detector=41, and expId=3020110200190.  The calibration data and postISRCCD images are in 

      /project/shared/BOT/rerun/cslage/PTC_LSSTCAM_12673

      I queried the number of good pixels (good meaning the mask plane is 0) by amp, and obtained the following:

      C10 0
      C11 0
      C12 0
      C13 0
      C14 0
      C15 0
      C16 0
      C17 0
      C07 3539
      C06 614
      C05 11
      C04 78
      C03 11
      C02 20657
      C01 974784
      C00 409729

      There should be about 1 million good pixels/amp, so only C01 looks normal.  I then compared mask plane and image plane in a good region between C01(good) and C06(bad):

      C06 Mask Good Region [6 6 6 6 6]
      C06 Image Good Region [99972.336 99972.336 99972.336 99972.336 99972.336]
      C01 Mask Good Region [0 0 0 0 0]
      C01 Image Good Region [104243.4   106106.586 105608.06  105036.266 104154.09 ]
       
      dict_items([('BAD', 0), ('CR', 3), ('DETECTED', 5), ('DETECTED_NEGATIVE', 6), ('EDGE', 4), ('INTRP', 2), ('NO_DATA', 8), ('SAT', 1), ('SUSPECT', 7), ('UNMASKEDNAN', 9)])

      So the bad pixels have mask plane=6, which is 'SAT', and 'INTRP', so they have been interpreted as saturated.  This isn't right, beacuse at ~100K ADU, they shouldn't be saturated.  So I looked at obs_lsst/polict/lsstCam/R11.yaml for S12, and it shows:

          S12 :
            C10 : { gain : 0.699516, readNoise : 4.745807, saturation : 135968.062500 }
            C11 : { gain : 0.699235, readNoise : 4.708802, saturation : 136083.000000 }
            C12 : { gain : 0.695590, readNoise : 4.748249, saturation : 135391.828125 }
            C13 : { gain : 0.690748, readNoise : 4.987112, saturation : 134143.984375 }
            C14 : { gain : 0.696782, readNoise : 5.019731, saturation : 135183.234375 }
            C15 : { gain : 0.689901, readNoise : 5.063357, saturation : 134983.390625 }
            C16 : { gain : 0.693390, readNoise : 4.898793, saturation : 135653.421875 }
            C17 : { gain : 0.698506, readNoise : 4.976052, saturation : 135490.921875 }
            C07 : { gain : 0.721271, readNoise : 4.937364, saturation : 135981.453125 }
            C06 : { gain : 0.715233, readNoise : 4.710922, saturation : 134819.281250 }
            C05 : { gain : 0.713144, readNoise : 4.725629, saturation : 134069.796875 }
            C04 : { gain : 0.713045, readNoise : 4.748403, saturation : 134037.093750 }
            C03 : { gain : 0.711816, readNoise : 4.717724, saturation : 133621.531250 }
            C02 : { gain : 0.717907, readNoise : 4.945924, saturation : 135765.281250 }
            C01 : { gain : 0.775595, readNoise : 6.067882, saturation : 160979.125000 }
            C00 : { gain : 0.725844, readNoise : 5.204304, saturation : 135903.109375 }

      They are being flagged as saturated, because the gain is listed as ~0.7 (too low, it is actually more like 1.1).  For C06, 134819*0.715=96395, so anything above this is flagged as saturated.  For C01, 160979*0.775=131036, so this is why C01 is not flagged as saturated.  So we need to do two things:
      (1) This will happen even with the right values, so we need to leave the test for too few pixels in ptc.py.  this is the line at: https://github.com/lsst/cp_pipe/blob/28eb519a8f97a5f2db6c21412e9172b7e0c0f463/python/lsst/cp/pipe/ptc.py#L683, but we need to increase the test to np.sum(w) < 10000 , or some reasonable number.
      (2) We need to update the gain, noise, and sat level values in the Rxx.yaml files.  i am willing to work to get the right values in there.

        Attachments

          Activity

          Hide
          mfisherlevine Merlin Fisher-Levine added a comment -

          In addition to the above (which are very necessary in their own right), I'd argue that the right fix for this case is almost certainly changing the ISR defaults for the PTC task to not do any interpolation. We fully expect this to be happening on PTC curves, and need to be able to make measurements of where it's actually happening.

          Without that change in defaults, we'll only ever be able to measure lower and lower saturation values...

           

          Show
          mfisherlevine Merlin Fisher-Levine added a comment - In addition to the above (which are very necessary in their own right), I'd argue that the right fix for this case is almost certainly changing the ISR defaults for the PTC task to not do any interpolation. We fully expect this to be happening on PTC curves, and need to be able to make measurements of where it's actually happening. Without that change in defaults, we'll only ever be able to measure lower and lower saturation values...  
          Hide
          cslage Craig Lage added a comment -

          Thanks Merlin.  An excellent point.  Let me try re-doing the ISR without interpolation and see if that fixes the problem.

          Show
          cslage Craig Lage added a comment - Thanks Merlin.  An excellent point.  Let me try re-doing the ISR without interpolation and see if that fixes the problem.
          Hide
          cslage Craig Lage added a comment -

          Setting doSaturation=False in the ISR seems to have fixed this.  The same CCD and expId as above now gives the number of good pixels by amp below.  I'll run some more tests, but this is clearly the right approach.

          C10 974426
          C11 974446
          C12 974814
          C13 974982
          C14 974602
          C15 974891
          C16 974624
          C17 974101
          C07 974814
          C06 974331
          C05 974850
          C04 974716
          C03 974722
          C02 974675
          C01 974784
          C00 974564
          

          Show
          cslage Craig Lage added a comment - Setting doSaturation=False in the ISR seems to have fixed this.  The same CCD and expId as above now gives the number of good pixels by amp below.  I'll run some more tests, but this is clearly the right approach. C10 974426 C11 974446 C12 974814 C13 974982 C14 974602 C15 974891 C16 974624 C17 974101 C07 974814 C06 974331 C05 974850 C04 974716 C03 974722 C02 974675 C01 974784 C00 974564
          Hide
          mfisherlevine Merlin Fisher-Levine added a comment -

          That's great news. I can't see how Christopher Waters or Andrés Alejandro Plazas Malagón would object to that being the default either, so if they confirm, we'll get it switched over so you don't have to add that.

          Show
          mfisherlevine Merlin Fisher-Levine added a comment - That's great news. I can't see how Christopher Waters or Andrés Alejandro Plazas Malagón would object to that being the default either, so if they confirm, we'll get it switched over so you don't have to add that.
          Hide
          cslage Craig Lage added a comment -

          I don't know if we can switch it over.  With the refactoring, the ISR task is now separate from the ptc task.  So the ISR task has no way of knowing that you are doing the ISR in preparation for a ptc task.  So I think you have to specify it.

          Show
          cslage Craig Lage added a comment - I don't know if we can switch it over.  With the refactoring, the ISR task is now separate from the ptc task.  So the ISR task has no way of knowing that you are doing the ISR in preparation for a ptc task.  So I think you have to specify it.
          Hide
          mfisherlevine Merlin Fisher-Levine added a comment -

          I no longer know how to do it in Gen3, but I'm >95% sure that's not correct.

          I think if you have pre-existing postISRCCDs available from running ISR yourself, such that the Quantum Graph generator skips that step, then yes, you're likely correct.

          However, if running the task without the pre-existing postISRCCDs available, the graph generator will run ISR itself, and in that mode, should be able to apply whatever defaults we specify.

          Christopher Waters could you just confirm/deny whether my rudimentary understanding of Gen3 is correct please?

          Show
          mfisherlevine Merlin Fisher-Levine added a comment - I no longer know how to do it in Gen3, but I'm >95% sure that's not correct. I think if you have pre-existing postISRCCDs available from running ISR yourself, such that the Quantum Graph generator skips that step, then yes, you're likely correct. However, if running the task without the pre-existing postISRCCDs available, the graph generator will run ISR itself, and in that mode, should be able to apply whatever defaults we specify. Christopher Waters could you just confirm/deny whether my rudimentary understanding of Gen3 is correct please?
          Hide
          cslage Craig Lage added a comment -

          Well, I'm running it with gen2, so maybe I'm wasting my time.

          Show
          cslage Craig Lage added a comment - Well, I'm running it with gen2, so maybe I'm wasting my time.
          Hide
          mfisherlevine Merlin Fisher-Levine added a comment -

          In Gen2 it's easy - give me 30s...

          Show
          mfisherlevine Merlin Fisher-Levine added a comment - In Gen2 it's easy - give me 30s...
          Hide
          mfisherlevine Merlin Fisher-Levine added a comment -

          OK, I'm wrong - in Gen2 in this task it's impossible as you say, but that's just because we're half way through the refactor for Gen3, so this task no longer is capable of running its own isr. This is one of the downsides of being in limbo, sorry.

          Show
          mfisherlevine Merlin Fisher-Levine added a comment - OK, I'm wrong - in Gen2 in this task it's impossible as you say, but that's just because we're half way through the refactor for Gen3, so this task no longer is capable of running its own isr. This is one of the downsides of being in limbo, sorry.
          Hide
          mfisherlevine Merlin Fisher-Levine added a comment -

          Point remains though (assuming I'm right above, which I think I have to be, otherwise there are huge problems in Gen3) that this task's "I'm going to run ISR for myself" config needs to turn that off.

          Show
          mfisherlevine Merlin Fisher-Levine added a comment - Point remains though (assuming I'm right above, which I think I have to be, otherwise there are huge problems in Gen3) that this task's "I'm going to run ISR for myself" config needs to turn that off.
          Hide
          czw Christopher Waters added a comment -

          The gen3 pipeline defines the ISR stage as well as the PTC stage (as gen3 pipelines are expected to be something like an "end-to-end" process).  For development, both gen2 and gen3 should allow you to run ISR once, and then iterate PTC calls as  many times as needed on the same set of postISRCCDs.  There should be a `cp_pipe/config/ptcIsr.py` that can be passed to `runIsr.py` to generate the postISRCCD files in the way expected by the PTC code.

          Show
          czw Christopher Waters added a comment - The gen3 pipeline defines the ISR stage as well as the PTC stage (as gen3 pipelines are expected to be something like an "end-to-end" process).  For development, both gen2 and gen3 should allow you to run ISR once, and then iterate PTC calls as  many times as needed on the same set of postISRCCDs.  There should be a `cp_pipe/config/ptcIsr.py` that can be passed to `runIsr.py` to generate the postISRCCD files in the way expected by the PTC code.
          Hide
          plazas Andrés Alejandro Plazas Malagón added a comment - - edited

          Christopher Waters That file (ptcIsr.py), along with the corresponding file in cp_pipe/pipelines, is in DM-23159, but not in master: https://github.com/lsst/cp_pipe/blob/tickets/DM-23159/config/ptcIsr.py. In both files, doSaturationInterpolation is already False.

          For this ticket, don't we need to set that parameter to false in the ISR config options? https://github.com/lsst/ip_isr/blob/master/python/lsst/ip/isr/isrTask.py#L749

          Show
          plazas Andrés Alejandro Plazas Malagón added a comment - - edited Christopher Waters That file ( ptcIsr.py ), along with the corresponding file in cp_pipe/pipelines , is in DM-23159 , but not in master: https://github.com/lsst/cp_pipe/blob/tickets/DM-23159/config/ptcIsr.py . In both files, doSaturationInterpolation is already False. For this ticket, don't we need to set that parameter to false in the ISR config options? https://github.com/lsst/ip_isr/blob/master/python/lsst/ip/isr/isrTask.py#L749
          Hide
          cslage Craig Lage added a comment - - edited

          According to the documentation, it is more important to have doSaturation=False, than doSaturationInterpolation=False.  If doSaturation=False, then no pixels are flagged as saturated, so there is nothing to interpolate, so it doesn't matter if doSaturationInterpolation is True or False.  However, if doSaturation=True and do SaturationInterpolation=False, the pixels will still be flagged as saturated, and so they will still be considered Bad, since they will have a mask value of 2 and not 0.

          Show
          cslage Craig Lage added a comment - - edited According to the documentation, it is more important to have doSaturation=False, than doSaturationInterpolation=False.  If doSaturation=False, then no pixels are flagged as saturated, so there is nothing to interpolate, so it doesn't matter if doSaturationInterpolation is True or False.  However, if doSaturation=True and do SaturationInterpolation=False, the pixels will still be flagged as saturated, and so they will still be considered Bad, since they will have a mask value of 2 and not 0.
          Hide
          czw Christopher Waters added a comment -

          Can the config file be taken out of DM-23159 and put on this ticket (with the addition of the `doSaturation=False` update)?  This avoids the problem of poorly defined saturation values here, and I think (please confirm!) that in the case of data that is truly saturated, simply adds additional points beyond the PTC turnoff that will be excluded anyway.

          Show
          czw Christopher Waters added a comment - Can the config file be taken out of DM-23159 and put on this ticket (with the addition of the `doSaturation=False` update)?  This avoids the problem of poorly defined saturation values here, and I think (please confirm!) that in the case of data that is truly saturated, simply adds additional points beyond the PTC turnoff that will be excluded anyway.
          Hide
          cslage Craig Lage added a comment - - edited

          For the record, here are the ISR configuration options I have been running.  Let me know if there are others that should be changed.  I think ultimately we will want doCrosstalk=True, but at this point I don't think we have good enough crosstalk coefficients to do that, and it is probably a small correction anyway.

          isr.doBias=True isr.doDark=True isr.doFringe=False isr.doSuspect=True isr.doDefect=True isr.overscan.fitType=MEDIAN_PER_ROW isr.overscan.order=1 isr.doFlat=False isr.doSaturation=False isr.doCrosstalk=False isr.edgeMaskLevel=AMP isr.numEdgeSuspect=10
          

          Show
          cslage Craig Lage added a comment - - edited For the record, here are the ISR configuration options I have been running.  Let me know if there are others that should be changed.  I think ultimately we will want doCrosstalk=True, but at this point I don't think we have good enough crosstalk coefficients to do that, and it is probably a small correction anyway. isr.doBias=True isr.doDark=True isr.doFringe=False isr.doSuspect=True isr.doDefect=True isr.overscan.fitType=MEDIAN_PER_ROW isr.overscan.order= 1 isr.doFlat=False isr.doSaturation=False isr.doCrosstalk=False isr.edgeMaskLevel=AMP isr.numEdgeSuspect= 10
          Hide
          plazas Andrés Alejandro Plazas Malagón added a comment -

          I agree that doCrosstalk should be True. We can leave it like True as default, and set it explicitly to False when calling the ISR.

          I'm not sure if we need isr.doFringe=False isr.doSuspect=True in the file. Christopher Waters?

          These are the values that are in ptcIsr.py:

           
          # ISR for inputs of Photon Transfer Curve task
          config.isr.doWrite = True
          config.isr.doOverscan = True
          config.isr.doAssembleCcd = True
          config.isr.doBias = True
          config.isr.doVariance = True
          config.isr.doLinearize = True
          config.isr.doCrosstalk = True
          config.isr.doBrighterFatter = False
          config.isr.doDark = True
          config.isr.doStrayLight = False
          config.isr.doFlat = False
          config.isr.doFringe = False
          config.isr.doApplyGains = False
          config.isr.doDefect = True
          config.isr.doNanMasking: True
          config.isr.doInterpolate: True
          config.isr.doSaturation=False
          config.isr.doSaturationInterpolation = False
          config.isr.growSaturationFootprintSize = 0  # We want the saturation spillover: it's good signal.
          

          Show
          plazas Andrés Alejandro Plazas Malagón added a comment - I agree that doCrosstalk should be True. We can leave it like True as default, and set it explicitly to False when calling the ISR. I'm not sure if we need isr.doFringe=False isr.doSuspect=True in the file. Christopher Waters ? These are the values that are in ptcIsr.py:   # ISR for inputs of Photon Transfer Curve task config.isr.doWrite = True config.isr.doOverscan = True config.isr.doAssembleCcd = True config.isr.doBias = True config.isr.doVariance = True config.isr.doLinearize = True config.isr.doCrosstalk = True config.isr.doBrighterFatter = False config.isr.doDark = True config.isr.doStrayLight = False config.isr.doFlat = False config.isr.doFringe = False config.isr.doApplyGains = False config.isr.doDefect = True config.isr.doNanMasking: True config.isr.doInterpolate: True config.isr.doSaturation=False config.isr.doSaturationInterpolation = False config.isr.growSaturationFootprintSize = 0 # We want the saturation spillover: it's good signal.
          Hide
          cslage Craig Lage added a comment -

          My understanding was that we need doSuspect=True in order to have the edge masking.  I think the edge masking is important because the flux rolls off significantly near the edges.  I thought doSuspect=True, isr.edgeMaskLevel=AMP isr.numEdgeSuspect=10 were all necessary to make the edge masking happen.  Maybe I'm wrong. 

          Show
          cslage Craig Lage added a comment - My understanding was that we need doSuspect=True in order to have the edge masking.  I think the edge masking is important because the flux rolls off significantly near the edges.  I thought doSuspect=True, isr.edgeMaskLevel=AMP isr.numEdgeSuspect=10 were all necessary to make the edge masking happen.  Maybe I'm wrong. 
          Hide
          czw Christopher Waters added a comment -

          `doLinearize` can't be True if we're using the PTC dataset to remeasure the linearity.  That's probably more of a linearity issue than PTC, however, so leaving it true here makes sense.

          And I think `doDefect` enables the edge masking if the `numEdgeSuspect` configuration option is non-zero.

          Show
          czw Christopher Waters added a comment - `doLinearize` can't be True if we're using the PTC dataset to remeasure the linearity.  That's probably more of a linearity issue than PTC, however, so leaving it true here makes sense. And I think `doDefect` enables the edge masking if the `numEdgeSuspect` configuration option is non-zero.
          Hide
          mfisherlevine Merlin Fisher-Levine added a comment -

          Some minor comments on the config and that's it.

          Show
          mfisherlevine Merlin Fisher-Levine added a comment - Some minor comments on the config and that's it.
          Show
          plazas Andrés Alejandro Plazas Malagón added a comment - Latest Jenkins: https://ci.lsst.codes/blue/organizations/jenkins/stack-os-matrix/detail/stack-os-matrix/33124/pipeline/

            People

            Assignee:
            plazas Andrés Alejandro Plazas Malagón
            Reporter:
            cslage Craig Lage
            Reviewers:
            Merlin Fisher-Levine
            Watchers:
            Andrés Alejandro Plazas Malagón, Christopher Waters, Craig Lage, Merlin Fisher-Levine
            Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

              Dates

              Created:
              Updated:
              Resolved:

                Jenkins

                No builds found.