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

Optimized rescale methods #850

Merged
merged 1 commit into from
Nov 13, 2024

Conversation

JasperMartins
Copy link
Contributor

I have noticed that for larger numbers of samples, PriorDict.rescale becomes quite slow due to the flatten operation, which iterates over all entries with a (slow) native python for loop.

The following PR provides a relatively simple fix that should be able to handle anything rescale methods can reasonably throw at it. In my testing, for only one sample, the new version is roughly equivalent to the old version (if anything slightly faster already). For larger counts, the new method is significantly faster.

On a related note, would it not make sense to let the return value have the appropriate shape for rescales of more than one sample?

@ColmTalbot
Copy link
Collaborator

@mattpitkin IIRC you added the flatten to this. I recently came across this for another reason and would be glad to get rid of the flatten. I agree that it would be nice to return the correct shape, it might break some things, but I think it's worth trying and if nothing in the test suite breaks then I think it'll be quite painless to get in.

@JasperMartins
Copy link
Contributor Author

I pondered applying a larger scale change that could remove the necessity of the flatten entirely. As far as I can tell, the flatten is necessary because JointPriors return empty lists until all parameters have been requested....actually, I wonder: if the order of the requested keys in rescale is not correct, ie something like keys = (JointPrior:a, SomeOtherPrior,JointPrior:b), the rescaling would return the order keys = (SomeOtherPrior, JointPrior:a, JointPrior:b), right?

In any case, what one could do is keep track of the object returned by JointPrior.rescale and update it in-place once all keys have been requested. This could be done with either fully initialized numpy arrays or with python lists.

Copy link
Collaborator

@mattpitkin mattpitkin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy with the suggested change as is, and think it should cover the cases for which I originally added flatten.

I agree that a better long term solution would be tracking the expected shapes of the outputs.

@JasperMartins JasperMartins force-pushed the prior_dict_rescale_update branch from 74ed7b8 to ec7245e Compare November 12, 2024 16:21
@ColmTalbot ColmTalbot force-pushed the prior_dict_rescale_update branch from ec7245e to 54f6f53 Compare November 13, 2024 00:45
@ColmTalbot ColmTalbot merged commit 26ad7fb into bilby-dev:main Nov 13, 2024
10 checks passed
@mj-will mj-will added this to the 2.4.0 milestone Nov 13, 2024
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.

4 participants