Skip to content
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

Construct images without sanity checks in _convertMask function in the HSM module #1264

Closed
arunkannawadi opened this issue Dec 5, 2023 · 5 comments · Fixed by #1279
Closed
Labels
hsm Related to the galsim.hsm adaptive moments tools lsst Relevant to LSST specifically optimization/performance Related to the speed and/or memory consumption of some aspect of the code

Comments

@arunkannawadi
Copy link
Member

When measuring the moments using FindAdaptiveMom, I find that a significant chunk of time is taken by the _convertMask function, which arises from various Image instantiations. Given that this is an internal function, I expect that the inputs provided to create an Image instance would always pass these checks and can directly use the _Image.

For reference, 38 out of 186 seconds is spend in _convertMask when calling FindAdaptiveMom.

@arunkannawadi arunkannawadi added lsst Relevant to LSST specifically hsm Related to the galsim.hsm adaptive moments tools labels Dec 5, 2023
@rmjarvis
Copy link
Member

rmjarvis commented Dec 5, 2023

I think the main thing is probably that there are a couple of checks currently that the mask is sane (no negative values and not all zero). So probably adding an option to skip those will speed this up a lot.

@arunkannawadi arunkannawadi added the optimization/performance Related to the speed and/or memory consumption of some aspect of the code label Dec 5, 2023
@rmjarvis
Copy link
Member

rmjarvis commented Dec 5, 2023

Probably simplest would be to add a leading underscore version of this _FindAdaptiveMom that skips all the sanity checks and requires that object_image and weight are already the right dtype and everything.

@arunkannawadi
Copy link
Member Author

That sounds like the least disruptive approach to me (not saying that modifying _convertMask would have been), but I could see a user potentially calling FindAdaptiveMom with inconsistent inputs. I'm happy to code this up, quantify the performance improvements and raise a PR here.

@erykoff
Copy link
Contributor

erykoff commented Dec 6, 2023

An additional public interface to observed_e1 and observed_e2 would also be nice. And the current return for observed_shape should probably use the optimized _Shape rather than Shape.

@arunkannawadi arunkannawadi linked a pull request Mar 8, 2024 that will close this issue
@arunkannawadi
Copy link
Member Author

Closing this issue as completed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hsm Related to the galsim.hsm adaptive moments tools lsst Relevant to LSST specifically optimization/performance Related to the speed and/or memory consumption of some aspect of the code
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants