DM-39639: Refactor linearity task, add Astier spline-based fit, and add full tests. #202
Conversation
erykoff
force-pushed
the
tickets/DM-39639
branch
2 times, most recently
from
2e6e1ce
to
58a7257
Compare
python/lsst/cp/pipe/utils.py
Outdated
| to allow for different linearity coefficients with different | ||
| photodiode settings. The minimization is a least-squares | ||
| fit with the residual of | ||
| Sum[(S(mu_i) + mu_i)/(k_j * D_i) - 1]**2, where mu_i is the |
There was a problem hiding this comment.
S(mu_i) here is the spline model at mu_i, right? If not, then there's even more reason for it to be explicitly stated.
There was a problem hiding this comment.
Also, a reference (to paper or code) for the original Astier model is good to add for the future.
There was a problem hiding this comment.
So I can put in a link to the code, though I don't know how long it will live there.
python/lsst/cp/pipe/utils.py
Outdated
| Array of flat mean values. | ||
| mask : `np.ndarray` (M,), optional | ||
| Input mask (True is good point, False is bad point). | ||
| log : `blah`, optional |
python/lsst/cp/pipe/utils.py
Outdated
| self._pd = pd | ||
| self._mu = mu | ||
| self._grouping_values = grouping_values | ||
| self.log = log |
There was a problem hiding this comment.
self.log = log if log else logging.getLogger(__name__)? I see you check the existence of the logger below before writing to it, but it's easier to just have one.
python/lsst/cp/pipe/linearity.py
Outdated
| # to allow for different linearity coefficients with different | ||
| # photodiode settings. The minimization is a least-squares | ||
| # fit with the residual of | ||
| # Sum[(S(mu_i) + mu_i)/(k_j * D_i) - 1]**2, where mu_i is the |
There was a problem hiding this comment.
Same comment as below (utils.py/AstierSplineLinearityFitter) about the docstring/comment. This doesn't need a reference, but defining S is still useful.
python/lsst/cp/pipe/utils.py
Outdated
| Returns | ||
| ------- | ||
| ratio_models : `np.ndarray` (N,) | ||
| Model ratio, (mu_i + S(mu_i))/(k_j * D_i) |
tests/test_linearity.py
Outdated
| self.assertFloatsAlmostEqual( | ||
| linearizer.linearityCoeffs[amp_name][n_nodes:], | ||
| non_lin_spline_values, | ||
| atol=7.0, |
There was a problem hiding this comment.
Is this tolerance needed for all of the spline parameters? I'm less concerned about a 70/7 or 185/7 S/N, but a 1/7 would be concerning.
There was a problem hiding this comment.
It's not, it's just at the large ones. But the ratio comparison wasn't looking good. The proof is in the linearity of the residuals, but I wanted to add some sort of test here. If you think it's better to do different tolerances at low and high flux I can do that instead.
There was a problem hiding this comment.
Ok. If my specific worry isn't justified, then this is fine with me.
python/lsst/cp/pipe/utils.py
Outdated
| ratio[0] = 1.0 | ||
| p0[0: len(self._nodes)] = (ratio - 1) * self._nodes | ||
|
|
||
| # ratio_model3 = self.compute_ratio_model( |
There was a problem hiding this comment.
Add a comment explaining why if this should be retained.
|
|
||
| The fit has additional constraints to ensure that the spline | ||
| goes through the (0, 0) point, as well as a normalization | ||
| condition so that the average of the spline over the full |
There was a problem hiding this comment.
Can you explain this normalization condition more? Is this trying to conserve charge? I ask because if we attempted to spline fit something with a simple squared correction, it seems like this would cause problems.
There was a problem hiding this comment.
This is from Pierre's code, and what I think it's basically doing is what I said: the total sum of the spline term should be zero, so that the linear part is picked up by the linear fit. I think this is basically the same thing that happens in the squared correction as well, as it's a squared deviation from linear, but the linear term is dropped in the application.
There was a problem hiding this comment.
I've added this comment:
The fit has additional constraints to ensure that the spline
goes through the (0, 0) point, as well as a normalization
condition so that the average of the spline over the full
range is 0. The normalization ensures that the spline only
fits deviations from linearity, rather than the linear
function itself which is degenerate with the gain.
This makes sure that only relevant data gets input.
The outputs have been completely overhauled here and are fully tested.
erykoff
force-pushed
the
tickets/DM-39639
branch
from
9277341
to
db3d60a
Compare
No description provided.