# 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:
• Labels:
None
• Story Points:
1
• Team:
Data Release Production

#### 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.

#### Activity

Hide
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
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
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
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
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
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
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
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
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
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
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
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
Craig Lage added a comment -

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

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

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

Show
Merlin Fisher-Levine added a comment - In Gen2 it's easy - give me 30s...
Hide
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
Merlin Fisher-Levine added a comment -

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

Show
Merlin Fisher-Levine added a comment - Some minor comments on the config and that's it.
Hide
Andrés Alejandro Plazas Malagón added a comment -
Show
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:
Andrés Alejandro Plazas Malagón
Reporter:
Craig Lage
Reviewers:
Merlin Fisher-Levine
Watchers:
Andrés Alejandro Plazas Malagón, Christopher Waters, Craig Lage, Merlin Fisher-Levine