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

Performance improvement of npairs_s_mu_engine.pyx #623

Merged
merged 1 commit into from
Aug 9, 2016
Merged

Performance improvement of npairs_s_mu_engine.pyx #623

merged 1 commit into from
Aug 9, 2016

Conversation

johannesulf
Copy link
Contributor

@johannesulf johannesulf commented Aug 9, 2016

I noticed that s_mu_tpcf from from halotools.mock_observables is extremely slow and found that npairs_s_mu_engine.pyx had a very inefficient loop structure. I made small changes to the engine. s_mu_tpcf is now ~3 times faster when using 20 bins in s and mu. Previously, it took ~30 sec for a zheng07 mock (threshold -19) in bolplanck and now takes ~10 sec.

I have checked that the engine (and npairs_s_mu_engine.pyx) give identical results for this particular, non-trivial use case. I did not perform any other tests.

@aphearin
Copy link
Contributor

aphearin commented Aug 9, 2016

Not sure I follow this at a glance, @johannesulf. Do you know why your changes to the logic only apply to npairs_s_mu, and not, for example, npairs_rp_pi?

@johannesulf
Copy link
Contributor Author

@aphearin Sorry, I made an error with the time it took with the original code. The performance improvement is not nearly as dramatic as originally described. But the new code still makes it faster by a factor of a ~3 for my use case.

The same logic also improves npairs_rp_pi. It does crucially depend on the number of bins, of course. For 40 bins in rp and pi I also get an improvement of order ~2 for tpcf_rp_pi.

@aphearin
Copy link
Contributor

aphearin commented Aug 9, 2016

Ok, well this is passing all tests so I'll go ahead with the merge. Nice work with the performance enhancement. I'll raise an issue to adapt this for rp_pi purposes.

@aphearin aphearin merged commit 85af7f8 into astropy:master Aug 9, 2016
@johannesulf
Copy link
Contributor Author

Just to update what I did wrong: I originally compared the performance to halotools v0.3. That did not include commit 2e1caba. The speed increase of ~100 came largely from that commit.

@johannesulf johannesulf deleted the s_mu_engine_improve branch January 31, 2017 16:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants