-
Notifications
You must be signed in to change notification settings - Fork 659
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 improvements #4089
performance improvements #4089
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## develop #4089 +/- ##
===========================================
- Coverage 93.59% 93.59% -0.01%
===========================================
Files 192 192
Lines 25135 25133 -2
Branches 4056 4056
===========================================
- Hits 23525 23523 -2
Misses 1092 1092
Partials 518 518
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
@richardjgowers @orbeckst Hello! Can anybody review my PR? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems reasonable! Would you be able to see if this makes a change to the benchmarks?
See: https://github.com/MDAnalysis/mdanalysis/blob/develop/benchmarks/README.rst
@hmacdope Yes. I will get back to you with the result. |
@hmacdope I ran the benchmarks 3 times and got the following results: |
Hi, @hmacdope I made changes to more files where using np.einsum in place of np.sum seemed logical on a different branch and got the following benchmark results: |
The decrease in performance in some cases is unexpected. Did you have stuff running in the background at the same time @g2707 ? |
@hmacdope No, I did not start any new applications during the benchmark. |
@g2707 I can try on my computer tonight |
@hmacdope Should I add the new file changes before tonight? |
Yep! |
Linter Bot Results:Hi @g2707! Thanks for making this PR. We linted your code and found the following: Some issues were found with the formatting of your code.
Please have a look at the Please note: The |
Hi! @hmacdope Did you get a chance to run the benchmarks? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You will need a CHANGELOG
entry (newest first), to add yourself to AUTHORS
and as I think you are a new contributor we ask you to introduce yourself on the developer mailing list. ! Other than that looks good and all tests pass, just a few nitpicks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me! Introduce yourself on the developer mailing list and then I can merge. :)
@hmacdope Done! |
Fixes #2063
Changes made in this Pull Request:
PR Checklist
📚 Documentation preview 📚: https://readthedocs-preview--4089.org.readthedocs.build/en/4089/