DM-40487: Add uncertainty to the output of DipoleFitTask#399
Conversation
parejkoj
force-pushed
the
tickets/DM-40487
branch
from
776784f to
42a6576
Compare
ebellm
left a comment
ebellm
left a comment
There was a problem hiding this comment.
A math error in uncertainty propagation, and I think it wouldn't be hard to add the covariance.
| fitResult.params['ycenNeg'].stderr) | ||
| centroid = measBase.CentroidResult((fitResult.params['xcenPos'] + fitResult.params['xcenNeg']) / 2, | ||
| (fitResult.params['ycenPos'] + fitResult.params['ycenNeg']) / 2., | ||
| math.sqrt(posCentroid.xErr**2 + negCentroid.xErr**2), |
There was a problem hiding this comment.
incorrect error propagation (see e.g. here)
| math.sqrt(posCentroid.xErr**2 + negCentroid.xErr**2), | |
| math.sqrt(posCentroid.xErr**2 + negCentroid.xErr**2)/2, |
There was a problem hiding this comment.
If you can access covar (have to confirm indices):
| math.sqrt(posCentroid.xErr**2 + negCentroid.xErr**2), | |
| math.sqrt(posCentroid.xErr**2 + negCentroid.xErr**2 + 2*fitResult.covar[0,1])/2., |
There was a problem hiding this comment.
Dang it, just forgot the multiplier! Thanks for the catch, though it makes the overall uncertainty even smaller compared to that from SdssCentroid.
I'd missed covar. It looks like scale_covar=True by default, so the covariance is scaled by the reduced chi2 see here, but I think all the uncertainties are similarly scaled, so it should be ok.
I was surprised that the x pos/neg and y pos/neg covariances are both negative (in the "really a dipole" test data), thus further reducing the overall centroid uncertainty compared with SdssCentroid. I don't have a good feeling for how much smaller is appropriate though.
There was a problem hiding this comment.
I'm not totally surprised the uncertainties end up smaller: we're fitting 3 images instead of just one as in SdssCentroid, so there's more information available to the fitter. But we can investigate further on DM-50839.
There was a problem hiding this comment.
In particular, the total centroid uncertainty (0.0088) is now less than the individual positive/negative uncertainties (0.013)! That may be correct, but it's still surprising to me.
| centroid = measBase.CentroidResult((fitResult.params['xcenPos'] + fitResult.params['xcenNeg']) / 2, | ||
| (fitResult.params['ycenPos'] + fitResult.params['ycenNeg']) / 2., | ||
| math.sqrt(posCentroid.xErr**2 + negCentroid.xErr**2), | ||
| math.sqrt(posCentroid.yErr**2 + negCentroid.yErr**2)) |
There was a problem hiding this comment.
| math.sqrt(posCentroid.yErr**2 + negCentroid.yErr**2)) | |
| math.sqrt(posCentroid.yErr**2 + negCentroid.yErr**2))/2 |
| (fitParams['ycenPos'] + fitParams['ycenNeg']) / 2.) | ||
| dx, dy = fitParams['xcenPos'] - fitParams['xcenNeg'], fitParams['ycenPos'] - fitParams['ycenNeg'] | ||
| # TODO: We could include covariances, which could be derived from | ||
| # `fitResult.params[name].correl`, but those are correlations. |
There was a problem hiding this comment.
fitResult should have a covar parameter according to this that would provide the covariance matrix--no need to use correl.
parejkoj
force-pushed
the
tickets/DM-40487
branch
from
42a6576 to
561faa1
Compare
|
Ok, covariances added on a new commit. |
No need to extract fields, just use the record itself; makes for easier test debugging.
* Cleanup the return struct from `fitDipole` to use CentroidResult, so that we can just put that in the source record directly. * I'm not keen on how I'm using the SDSS Centroid uncertainties as the test expected values, but I don't know a better approach. * Not including the correlations/covariances, as that math is trickier.
Add centroids and uncertainties. Use plt.subplots to cleanup display2dArray with better labels.
lmfit now prefers `params` in the MinimizerResult, which include both the best fit `value` and the `stderr` (and other things). Switching to this makes it less confusing, since I needed to use `fitResult.params` in addition to the existing `fitParams`, which were not at all the same thing!
This makes it more explicit that the positive/negative/total flux values are going to be the same unless separateNegParams is set, which it is not by default from the config.
parejkoj
force-pushed
the
tickets/DM-40487
branch
from
561faa1 to
6d86b0c
Compare
2 participants
No description provided.