DM-32470: Dark verification in OCPS calibration script from DM-31897 fails because NOISe is too low with respect nominal readnoise #9
Conversation
| @@ -37,6 +37,7 @@ def setDefaults(self): | |||
| 'NOISE': 'STDEVCLIP', } | |||
|
|
|||
| self.crImageStatKeywords = {'CR_NOISE': 'STDEV', } # noqa F841 | |||
| self.metadataStatKeywords = {'RESIDUAL STDEV': 'AMP', } # noqa F841 | |||
There was a problem hiding this comment.
Other keys, such as CR_NOISE, have _ in between words. Would it be better to also add an underscore here to be consistent?
There was a problem hiding this comment.
This comes from a choice in ip_isr. I wanted to keep changes out of that. I had thought about moving this from the task metadata to the exposure metadata, and then making the metadata stats code look there instead of the task metadata (removing that connection). However, this wouldn't be available for previously processed data, and so there'd always be two versions.
This was a design decision, and I'm still not sure I made the right one.
| amp = detector[ampName] | ||
| verify['NOISE'] = bool(np.abs(stats['NOISE'] - amp.getReadNoise())/amp.getReadNoise() <= 0.05) | ||
| readNoise = detector[ampName].getReadNoise() | ||
| if 'RESIDUAL STDEV' in metadataStats and ampName in metadataStats['RESIDUAL STDEV']: |
There was a problem hiding this comment.
Should we add a comment here, or above, explaining RESIDUAL STDEV more? For example, saying that it is the standard deviation of the overscan region after the overscan has been removed., and that this should be identical to the true read noise (according to an explanation I got from you once). This given that the name doesn't seem to be that descriptive.
There was a problem hiding this comment.
Explanation added here and in verifyDark. Minor differences also removed in that check (dark checked that overscanReadNoise was found, bias did not).
| verify['NOISE'] = bool(np.abs(stats['NOISE'] - amp.getReadNoise())/amp.getReadNoise() <= 0.05) | ||
| readNoise = detector[ampName].getReadNoise() | ||
| if 'RESIDUAL STDEV' in metadataStats and ampName in metadataStats['RESIDUAL STDEV']: | ||
| overscanReadNoise = metadataStats['RESIDUAL STDEV'][ampName] |
There was a problem hiding this comment.
OK, here you assign the per amp RESIDUAL STDEV to a variable with a more descriptive name, but maybe a comment would still be good.
python/lsst/cp/verify/verifyBias.py
Outdated
| if verify['SUCCESS'] is False: | ||
| success = False | ||
| # This isn't something to fail on yet. |
There was a problem hiding this comment.
So should this actually be a TODO with an associated ticket, or should we provide more information? I'm trying to think of our future selves when we come back to this part of the code and wonder about this (i.e., when/if should we fail on this?)
There was a problem hiding this comment.
I've updated the comment. I think we would only want to fail on this if we implement versioned cameras.
| amp = detector[ampName] | ||
| verify['NOISE'] = bool(np.abs(stats['NOISE'] - amp.getReadNoise())/amp.getReadNoise() <= 0.05) | ||
| readNoise = detector[ampName].getReadNoise() | ||
| if 'RESIDUAL STDEV' in metadataStats and ampName in metadataStats['RESIDUAL STDEV']: |
There was a problem hiding this comment.
Same comments in this "Dark" file as in the "Bias" one
| """Calculate quality statistics and verify they meet the requirements | ||
| for a calibration. | ||
|
|
||
| Parameters | ||
| ---------- | ||
| inputExp : `lsst.afw.image.Exposure` | ||
| The ISR processed exposure to be measured. | ||
| taskMetadata : `lsst.daf.base.PropertySet`, optional |
There was a problem hiding this comment.
Is RESIDUAL STDEV the only statistic from the metadata that we currently use? Are there any other statistics in the metadata that could be useful? Where would we need to look if we wanted to see all the contents of this metadata object to see if there's something else that might be useful?
There was a problem hiding this comment.
It's produced by IsrTask, and contains a variety of per-amp measurements at different stages of processing. It also holds some statistics about the overscan. It's stored there because when I reworked the task, I wanted to save some of the information that we were measuring that wasn't being saved anywhere. This gets back to the design decision comment above; it's probably worth thinking about if this information should be written to the exposure metadata as well.
There was a problem hiding this comment.
Maybe we can discuss this more in one of our meetings, especially if we decide that we want to use these statistics as stability metrics or something like that. In the end, I'm just concerned about 1) are we saving potentially useful information in this metadata, 2) if so, how can we easily access it for plotting/use/further analysis?
This change adds the possibility of pulling values from the task metadata. This code path is then used to pull the measured scatter in the overscan as an estimate the read noise. This should resolve the issue of the camera read noise not matching the quoted camera value.