DM-27458: FULLCOVARIANCE in PTC task is rejecting more points than it should for some BOT data detectors #65
Conversation
There was a problem hiding this comment.
I'm concerned that the PTC code is becoming increasingly difficult to understand. I'm hopeful that the gen3 rewrite will help somewhat.
I'd also suggest rebasing the existing commits into a simpler set that clearly define what changes have been made. It might also be good to move the travis/lint change to either the beginning or end of the commit chain so the PTC commits are together.
tests/test_ptc.py
Outdated
| @@ -325,7 +327,7 @@ def test_getInitialGoodPoints(self): | |||
| for i, factor in enumerate([-0.5, -0.1, 0, 0.1, 0.5]): | |||
| newYs[-1] = ys[-1] + (factor*ys[-1]) | |||
| points = self.defaultTask._getInitialGoodPoints(xs, newYs, 0.05, 0.25) | |||
| assert (np.all(points[0:-1]) == True) # noqa: E712 - flake8 is wrong here because of numpy.bool | |||
| assert (np.all(points[0:-2]) == True) # noqa: E712 - flake8 is wrong here because of numpy.bool | |||
There was a problem hiding this comment.
This doesn't need the == True, as np.all returns either True or False, and assert expects either True or False. This should also remove the need for the flake8 exception.
| @@ -684,6 +680,9 @@ def measureMeanVarCov(self, exposure1, exposure2, region=None, covAstierRealSpac | |||
| wDiff = np.where(diffIm.getMask().getArray() == 0, 1, 0) | |||
| w = w12*wDiff | |||
|
|
|||
| if np.sum(w) == 0: | |||
| return np.nan, np.nan, None | |||
There was a problem hiding this comment.
Why doesn't this return np.nan, np.nan, np.nan? The docstring says that's the failure mode. Should this return an empty list instead?
There was a problem hiding this comment.
This is the same failure mode that we have a few lines above when {mu1} or {mu2} ara NaN. I will update the docstring.
python/lsst/cp/pipe/ptc.py
Outdated
| Too low and the non-linear points will be excluded, biasing the NL fit.""" | ||
| Too low and the non-linear points will be excluded, biasing the NL fit. | ||
|
|
||
| This function also masks points after the variance estarts dcreasing. |
| @@ -236,12 +235,13 @@ class CovFit: | |||
| Maximum range to select from tuple. | |||
| """ | |||
|
|
|||
| def __init__(self, inputTuple, maxRangeFromTuple=8): | |||
| def __init__(self, inputTuple, maxRangeFromTuple=8, expIdMask=None): | |||
There was a problem hiding this comment.
expIdMask needs to be listed in the parameters above. I'm also not sure if None is the correct default. I think the usage here will cause an error ('list indices must be integers or slices, not NoneType').
Also, if this is unrelated to the expIdMask used elsewhere (which is a dict containing amp names), can this be called something else?
| @@ -424,16 +425,15 @@ def getFitDataFromCovariances(i, j, mu, fullCov, fullCovModel, fullCovSqrtWeight | |||
| covarianceModel = fullCovModel[:, i, j]*(gain**2) | |||
| weights = fullCovSqrtWeights[:, i, j]/(gain**2) | |||
|
|
|||
| # select data used for the fit | |||
| mask = weights != 0 | |||
| maskFromWeights = weights != 0 | |||
| if returnMasked: | |||
There was a problem hiding this comment.
Is this case ever called now? Am I mistaken that the turnoff based masking is the only masking used?
There was a problem hiding this comment.
It's not called now. Now we do use the final mask that results after doing the {EXPAPPROXIMATION} fit before this {FULLCOVARIANCE} fit. This final mask has cuts from outlier rejection (in getInitialGoodPoints) and iterations ("newMask" in the code), in addition to the turn-off based masking (which is included in the detInitialGoodPoints function).
I decided to leave this original weighting scheme from the Astier code in case that we want to use it (so I change the name of the variable slightly), but we may decide in the future that we want to get rid of this if we don't use it at all to make things easier to read.
Remove iterations from full fit Pass expIdMask[amp] to CovFit objects Adjust plots due to new mask Remove config options not needed Fix docstrings Rename expMaskId in CovFit class Fix tests Remove unnecessary True in assert
Move pivot calculation to getInitialGoodPoints
plazas
force-pushed
the
tickets/DM-27458
branch
from
81c5c6e
to
f4cdeba
Compare
No description provided.