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

Improving npairs(s, mu) performance and testing. #768

Merged
merged 18 commits into from
Jun 13, 2017

Conversation

manodeep
Copy link
Contributor

@manodeep manodeep commented Jun 6, 2017

Completely un-tested, un-benchmarked. I am assuming there are existing tests for the npairs(s, mu) pair counters.

Related to #763

@duncandc
Copy link
Contributor

Performance is about 10x's worse for me when applying this code. Are there some compiling flags that we need implement?

@aphearin
Copy link
Contributor

Cython's automated compiler flags are sufficiently good that a 10x improvement never occurs by simply adding flags. If you just run cython -a on the module, it showed that there were a couple of forgotten declarations that I just added just now.

@duncandc
Copy link
Contributor

Excellent. That did the trick. It looks to be of order 50% performance increase over the code in the repo right now. Im going to push my modifications to the pair counters and tests to this PR.

@duncandc duncandc changed the title Improving npairs(s, mu) performance Improving npairs(s, mu) performance and testing. Jun 12, 2017
@duncandc
Copy link
Contributor

Okay, I plan on merging this with master after it passes the travis build. I think the brute force pairs.s_mu_npairs comparison test should alleviate any concern that may have been lingering about the reliability of the optimized npairs_s_mu() function. I have also edited the doc strings of the s_mu pair counters.

@duncandc
Copy link
Contributor

@aphearin do you expect some travis builds to fail currently?

@duncandc
Copy link
Contributor

okay, note that doc strings with math need to be preceded by an 'r' in order to pass python 3 builds.

@duncandc duncandc merged commit ce11b77 into astropy:master Jun 13, 2017
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.

3 participants