Optimize bits of HSM code #1279
Conversation
Since in the normal mode, `mask` should have several non-zero pixels, it is faster to return with any than to sum over all the pixels.
conversion did not happen.
arunkannawadi
force-pushed
the
hsm_optimization
branch
from
536621f to
8e32318
Compare
| @property | ||
| def observed_shape(self): | ||
| return Shear(e1=self._data.observed_e1, e2=self._data.observed_e2) | ||
| return Shear(e1=self.observed_e1, e2=self.observed_e2) |
There was a problem hiding this comment.
@erykoff wanted this to use galsim._Shear here, but that requires conversion from e to g before we invoke _Shear and might not be worthwhile.
There was a problem hiding this comment.
If you want to optimize this too, you could try:
e = self._data.observed_e1 + 1j*self._data.observed_e2
g = e / (1 + np.sqrt(1.-abs(e)**2))
return _Shear(g)
I suspect that's a little faster, but might just be nibbling at the edges.
There was a problem hiding this comment.
In the LSST DM code base, the reason to get observed_shape was because there was no way to get observed_e1 directly up until now. Now that we can access them directly, I honestly don't think using _Shear is necessary.
| @property | ||
| def observed_shape(self): | ||
| return Shear(e1=self._data.observed_e1, e2=self._data.observed_e2) | ||
| return Shear(e1=self.observed_e1, e2=self.observed_e2) |
There was a problem hiding this comment.
If you want to optimize this too, you could try:
e = self._data.observed_e1 + 1j*self._data.observed_e2
g = e / (1 + np.sqrt(1.-abs(e)**2))
return _Shear(g)
I suspect that's a little faster, but might just be nibbling at the edges.
| hsmparams = HSMParams.check(hsmparams) | ||
| if check: | ||
| _checkWeightAndBadpix(gal_image, weight=weight, badpix=badpix) |
There was a problem hiding this comment.
You need to add tests of check=False. (For both functions.)
There was a problem hiding this comment.
Of course! This was not quite what we discussed in #1264 (we talked about going for a _FindAdaptiveMomView there) which is why I first wanted to run this by you before I wrote the tests for the two cases.
There was a problem hiding this comment.
Sure. This seems like a reasonable API to me.
| @property | ||
| def observed_shape(self): | ||
| return Shear(e1=self._data.observed_e1, e2=self._data.observed_e2) | ||
| return Shear(e1=self.observed_e1, e2=self.observed_e2) |
There was a problem hiding this comment.
In the LSST DM code base, the reason to get observed_shape was because there was no way to get observed_e1 directly up until now. Now that we can access them directly, I honestly don't think using _Shear is necessary.
galsim/hsm.py
Outdated
| @@ -779,6 +800,8 @@ def FindAdaptiveMom(object_image, weight=None, badpix=None, guess_sig=5.0, preci | |||
| ``GalSimHSMError`` exception if shear estimation fails. If set to | |||
| ``False``, then information about failures will be silently stored in | |||
| the output ShapeData object. [default: True] | |||
| check: Check if the object_image, weight are in the correct format and valid. | |||
There was a problem hiding this comment.
I missed out on badpix here and will include that as well.
arunkannawadi
force-pushed
the
hsm_optimization
branch
from
86984ba to
dcb27a8
Compare
|
OK, something completely unrelated seems to be failing. |
| @@ -1,6 +1,6 @@ | |||
| .. image:: https://github.com/GalSim-developers/GalSim/workflows/GalSim%20CI/badge.svg | |||
| .. image:: https://github.com/GalSim-developers/GalSim/workflows/GalSim%20CI/badge.svg?branch=main | |||
There was a problem hiding this comment.
I also updated this link to show the badge corresponding to the run on the main branch, typically after merging a feature branch. I don't suppose you want to show case that tests on a development branch failed in the repo's README.
|
For now, just pin pytest to <8. They changed something with pytest v8 about how they run (or don't!) setup() before the tests functions. I haven't dug in to figure out what to change to make it work with v8 yet. |
arunkannawadi
force-pushed
the
hsm_optimization
branch
from
dcb27a8 to
798581d
Compare
arunkannawadi
force-pushed
the
hsm_optimization
branch
from
798581d to
6f8bfe9
Compare
|
Test suite looks good. Thanks. |
rmjarvis
added
docs
This PR addresses a lot of the concerns raised in #1264. I also separated conversion code from code that checks things as much as possible. I can add some additional unit tests if this all looks sensible.