DM-33297: Add correction of systematic photodiode error #118
Conversation
mfisherlevine
changed the title
| config: | ||
| connections.inputPtc: ptc | ||
| connections.inputLinearizer: linearizer | ||
| connections.outputPhotodiodeCorrection: pdCorrection |
There was a problem hiding this comment.
Github complaint. Even though this doesn't cause a build error, it should still be fixed.
There was a problem hiding this comment.
Sorry, I don't understand what needs to be fixed here.
There was a problem hiding this comment.
It needs a newline at the end of the last line.
python/lsst/cp/pipe/linearity.py
Outdated
| continue | ||
|
|
||
| if (len(inputPtc.expIdMask[ampName]) == 0) or self.config.ignorePtcMask: | ||
| self.log.warning("Mask not found for %s in non-linearity fit. Using all points.", ampName) | ||
| self.log.warning("Mask not found for %s in detector %s in fit.Using all points.", |
python/lsst/cp/pipe/linearity.py
Outdated
| inputOrdinate = np.array(inputPtc.rawMeans[ampName])[mask] | ||
| if self.config.usePhotodiode: | ||
| # Here's where we bring in the photodiode data | ||
| # TODO: replace when pd data is ingested. |
There was a problem hiding this comment.
This should have a ticket reference after the TODO. I added DM-33586 for this work.
ETA: Thinking it through, this block may need to be moved to the PtcExtract code due to the way connections are handled.
There was a problem hiding this comment.
I added the reference to DM-33586.
| except (RuntimeError, OSError): | ||
| continue | ||
| if nExps > 0: | ||
| # The 5E8 factor bring the modExpTimes back to about |
There was a problem hiding this comment.
Does this need to be scaled? It is just for visualization?
There was a problem hiding this comment.
It's just for visualization, so the plots look similar to the plots vs exposure time. We can remove this factor if you think that's best.
There was a problem hiding this comment.
If you can note that in the comment, it's fine. I just wanted to make sure I wasn't missing something.
| (`lsst.ip.isr.IsrProvenance`). | ||
|
|
||
| Notes | ||
| ----- |
There was a problem hiding this comment.
Or here. Notes shouldn't be empty if the header is there, though.
There was a problem hiding this comment.
I added an explanation of the algorithm in the notes.
python/lsst/cp/pipe/linearity.py
Outdated
| @@ -354,7 +424,20 @@ def run(self, inputPtc, dummy, camera, inputDims): | |||
| linearizer.fitParams[ampName] = np.array(polyFit) | |||
| linearizer.fitParamsErr[ampName] = np.array(polyFitErr) | |||
| linearizer.fitChiSq[ampName] = chiSq | |||
|
|
|||
| linearizer.linearFit[ampName] = linearFit | |||
| residuals = inputOrdinate[fluxMask] - modelOrdinate | |||
There was a problem hiding this comment.
This line causes a build error from tests/test_ptc.py: "ValueError: operands could not be broadcast together with shapes (20,) (10,)".
Ok, it looks like the fluxMask gets reset by the threshold test, and that marks everything as good (the (20,)), and modelOrdinate is defined using the previous definition (it's the (10,)).
There was a problem hiding this comment.
OK. I need to study the testing some more. I'll work on it, but it won't be until the week of Feb 14.
There was a problem hiding this comment.
I see what's happening here, but we need to discuss the best fix. I had only tested it with linearity.Type='Spline', where this line works. The modelOrdinate is different lengths for 'Spline' than for the others. In the other fit types ('Polynomial', 'Squared', 'LookupTable'), the modelOrdinate is only as long as linearAbscissa, which is just the low flux part of the curve. In 'Spline', linearOrdinate is increased to the full length of the data with this line:
cp_pipe/python/lsst/cp/pipe/linearity.py
Line 259 in a4390a1
I think the spline fit is correct, but the other fits are not. The modelOrdinate should include the full length of the data like it does in 'Spline'. But maybe I am confused.
There was a problem hiding this comment.
Thinking about this some more, I think I see a simple fix, which I have pushed.
| config: | ||
| connections.inputPtc: ptc | ||
| connections.inputLinearizer: linearizer | ||
| connections.outputPhotodiodeCorrection: pdCorrection |
There was a problem hiding this comment.
It needs a newline at the end of the last line.
| except (RuntimeError, OSError): | ||
| continue | ||
| if nExps > 0: | ||
| # The 5E8 factor bring the modExpTimes back to about |
There was a problem hiding this comment.
If you can note that in the comment, it's fine. I just wanted to make sure I wasn't missing something.
python/lsst/cp/pipe/pdCorrection.py
Outdated
| Provenance data for the new calibration | ||
| (`lsst.ip.isr.IsrProvenance`). | ||
|
|
||
| Notes: |
There was a problem hiding this comment.
This needs to have the underline of five -, and no :.
inputLinearity in PhotodiodeCorrectionTask inputs. Ensure that the photodiodeCorrection is discarded when not needed.
…o added the pipelin yaml file and removed the exposure from the pdCorrection dimensions
mfisherlevine
force-pushed
the
tickets/DM-33297
branch
from
ff52054
to
d218e30
Compare
This ticket was to add the photodiode and photodiode systematic correction code to the linearizer.