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

Speed up centroid function (remove interpolation) #179

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jordiferrero
Copy link
Contributor

@jordiferrero jordiferrero commented Feb 2, 2023

Description of the change

I realised I had overengineered the com utils function. I have now simplified it with a huge speed improvement. It does not rely on scipy interpolation anymore.

Progress of the PR

  • Change implemented (can be split into several points),
  • docstring updated (if appropriate),
  • ready for review.

@jlaehne
Copy link
Contributor

jlaehne commented Feb 15, 2023

Indeed, the function runs through about 6-times faster on a map of mine. For the uniform axis, the results are the same - but when converting to non-uniform axes, the results are not the same as for the old implementation. In particular, for jacobian=False the centroid in the old implementation is the same in energy and wavelength scale, which makes sense. In the new implementation, they are not the same.

I think the weighted average that you calculate is correct for the equidistant scale, but for non-uniform axes you still would need interpolation if you want to use that formula.

@jlaehne jlaehne added this to the v0.2.2 milestone Feb 16, 2023
@jlaehne jlaehne modified the milestones: v0.2.2, v0.2.3 Mar 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants