DM-23045: Validate linearity correction #30
Conversation
python/lsst/cp/pipe/ptc.py
Outdated
| Parameters from n-order polynomial fit to meanSignalVector vs exposureTimeVector. | ||
|
|
||
| fracNonLinearityResidual : `list` of `float` | ||
| Fractial residuals from the meanSignal vs exposureTime curve with respect to linear part of |
There was a problem hiding this comment.
Fractial → Fractional
| signalIdeal = parsFit[0] + parsFit[1]*timeRange | ||
| signalUncorrected = self.funcPolynomial(parsFit, timeRange) |
There was a problem hiding this comment.
Good catch on fixing that, not sure how that was ever OK...
python/lsst/cp/pipe/ptc.py
Outdated
| return (polynomialLinearizerCoefficients, c0, linearizerTableRow, linResidual, parsFit, parsFitErr, | ||
| reducedChiSquaredNonLinearityFit) | ||
| fracNonLinearityResidual, reducedChiSquaredNonLinearityFit) |
There was a problem hiding this comment.
We have got to get away from making the return types massive tuples, it's just way too fragile.
Could you at least make this a named tuple so that people can't get it wrong, and code accessing components of the return won't be broken?
Also, just a general point: not putting the new returned item last is unnecessarily dangerous, as other code relying on the order will accidentally grab the wrong item here (if it doesn't crash because of it) whereas putting on the end wouldn't have that same effect.
There was a problem hiding this comment.
And actually, having looked further, I don't see any calls to calculateLinearityResidualAndLinearizers() in the tests, which means that this is likely a breaking change, and should have been caught, so could you add something that would test this bit of code please too?
There was a problem hiding this comment.
You could also create a quick dataclass instead of a named tuple. They are also light weight but much clearer to set up and use.
There was a problem hiding this comment.
Yeah, Tim is right, this has got dataclass written all over it.
tests/test_ptc.py
Outdated
| for i in np.arange(len(localDataset.nonLinearityResiduals[ampName])): | ||
| self.assertAlmostEqual(localDataset.nonLinearityResiduals[ampName][i], | ||
| self.inputNonLinearityResiduals[i]) | ||
| for i in np.arange(len(returnedDataset.nonLinearityResiduals[ampName])): |
There was a problem hiding this comment.
I wonder if there's a more elegant way of doing that?
There was a problem hiding this comment.
Maybe something like this?
for calc, truth in zip(returnedDataset.nonLinearityResiduals[ampName], inputNonLinearityResiduals): self.assertAlmostEqual(calc, truth)
No description provided.