New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
DM-26453: Add sqrt(var) as weight to EXPAPPROXIMATION and POLYNOMIAL fit residual in ptc.py #47
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very minor comments.
meanSignalVector, | ||
funcPolynomial) | ||
(parsFit, parsFitErr, | ||
reducedChiSquaredNonLinearityFit) = fitBootstrap(parsIniNonLinearity, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reducedChiSquaredNonLinearityFit
is an almost comically long variable name, but I guess it's preexisting...
python/lsst/cp/pipe/utils.py
Outdated
@@ -313,17 +316,23 @@ def fitLeastSq(initialParams, dataX, dataY, function): | |||
List with errors for fitted parameters. | |||
|
|||
reducedChiSqSingleLeastSquares : `float` | |||
Unweighted reduced chi squared | |||
Reduced chi squared |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whether it unweighted or not depends on if the optional weight parameter is actually passed, so maybe something like "Reduced chi squared, unweighted if weightsY
is not provided."
python/lsst/cp/pipe/utils.py
Outdated
@@ -374,15 +386,19 @@ def fitBootstrap(initialParams, dataX, dataY, function, confidenceSigma=1.): | |||
reducedChiSqBootstrap : `float` | |||
Reduced chi squared. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe amend this too so it's in keeping with the other one about the optional weighting? Or make the other like this, I think that's actually fine too.
python/lsst/cp/pipe/utils.py
Outdated
@@ -391,7 +407,7 @@ def errFunc(p, x, y): | |||
randomDelta = np.random.normal(0., sigmaErrTotal, len(dataY)) | |||
randomDataY = dataY + randomDelta | |||
randomFit, _ = leastsq(errFunc, initialParams, | |||
args=(dataX, randomDataY), full_output=0) | |||
args=(dataX, randomDataY, 1./np.sqrt(randomDataY)), full_output=0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like randomDataY
can be negative here - what happens then? I think you end up with a RuntimeWarning
and a bunch of nan
s in the return array, and then I'm not sure what happens downstream...
No description provided.