-
Notifications
You must be signed in to change notification settings - Fork 647
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 and refactored common compound code. #3005
Conversation
Hello @mnmelo! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
Comment last updated at 2021-02-08 05:22:51 UTC |
Codecov Report
@@ Coverage Diff @@
## develop #3005 +/- ##
===========================================
- Coverage 93.14% 93.08% -0.07%
===========================================
Files 171 171
Lines 22750 22706 -44
Branches 3218 3209 -9
===========================================
- Hits 21191 21135 -56
- Misses 1502 1514 +12
Partials 57 57
Continue to review full report at Codecov.
|
In #2376 I propose a solution to the cache invalidation problem, which is to cache at the |
fd8b073
to
cd85aea
Compare
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.
I like the DRY changes in this PR a lot. I can't see where the caching is, has this been removed for now? If so I'm happy to merge these changes.
I have dropped the caching for this PR, so it's more contained and easier to review. (Also, the caching invalidation mechanism is what is still up for discussion). Gonna clean up these lingering bugs and update soon (should've tested locally 😬). |
Performance increased and code duplication reduced.
cd85aea
to
4d4752c
Compare
Ok, this is ready for review. I guess codecov hit a roadbump or something because only |
To get my feet wet in |
Ping @richardjgowers. I think this is ready to merge. There seem to be some inconsistencies in Codecov computation triggering a fail there. |
@mnmelo thanks! |
* Optimized and refactored common compound code. Performance increased and code duplication reduced. * Added compound-splitting performance benchmarks
Fixes #3000
Changes made in this Pull Request:
core/group.py
refactored (more DRY now);Performance benchmarks, in the discussion of #3000, are quite promising, especially with caching.
Problem
Cached compound masks work great and can give large speedups, but invalidating them is tough: if I have an
AtomGroup
with cached masks for segments and the user moves a residue between segments those cached masks should no longer be valid, but how to notify an AtomGroup of something occurring at theResidueGroup
/SegmentGroup
level?Pointers appreciated.
PR Checklist