Multiple SED in ChromaticTransformation by absolute value of determinant#1349
Conversation
|
Looks good to me. This appears to be the only use of the determinant from |
|
May be we should make it |
|
If you could also simplify your how-to-reproduce code to mock up a |
| def detjac(w): | ||
| return np.linalg.det(np.asarray(self._jac(w)).reshape(2,2)) | ||
| sed *= detjac | ||
| sed *= np.abs(detjac) |
There was a problem hiding this comment.
This is not correct. abs needs to be inside the function. Note that detjac here is a function, not a number.
There was a problem hiding this comment.
Yes I didn't notice this at first. Moved inside the function.
There was a problem hiding this comment.
If we wanted to keep detjac as returning a signed number and to only apply the absolute value when doing the SED transformation, then the following should work
sed *= lambda w: np.abs(detjac(w))
Python could really do with better support for functional composition...
rmjarvis
left a comment
rmjarvis
left a comment
There was a problem hiding this comment.
LGTM. Thanks for the fix, Federico.
|
Credits go to Cole Meldorf (Penn) for identifying the existence of this issue. |
|
If we want to reproduce results from OU24 would one want to use the old behavior? Specifically, if one were modeling the PSF of an object in an image, is it necessary to reproduce the old behavior to recover the truth values for OU24? |
|
@wmwv I would hope not. If the Open Universe sims did anything that involved this bug, I think there would be huge problems in the images. Whatever code paths it used seem to have avoided this bug. |
|
Great, thank you for that reassurance. |
rmjarvis
added
numerics
5 participants
As described in #1346 1346, the
sed()function in theChromaticTransformationclass should multiply the SED by the absolute value of the determinant of the jacobian. It currently multiplies by the determinant, allowing for negative fluxes if the determinant is negative. The change here simply adds the absolute value when multiplying the SED.