-
Notifications
You must be signed in to change notification settings - Fork 646
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
Cache creation of compound masks #4612
base: develop
Are you sure you want to change the base?
Conversation
Hello @PicoCentauri! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found: There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻 Comment last updated at 2024-06-05 17:36:21 UTC |
Linter Bot Results:Hi @PicoCentauri! 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 |
db60a2a
to
2f537e4
Compare
self._cache[cache_key] = { | ||
"compound_indices": compound_indices, | ||
"data": (atom_masks, compound_masks, len(compound_sizes)) | ||
} |
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 am not sure if this is the canonical approach for caching. But the @cached
decorator does not work here I think.
2f537e4
to
e1b3b0b
Compare
e1b3b0b
to
1c393c9
Compare
@richardjgowers could you please have a look at this PR, looks like your wheelhouse. If you don't have the bandwidth please un-assign yourself and let me know. Thanks! |
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.
The TestUnwrapFlag.test_residues test fails. I assume that this is actually due to the changes here. What needs to be done to make the tests pass again?
this failure here:
the x coordinate is off by 55.423 where the boxlength on that dimension is 55.423, so that just looks like a rounding error. Those two x coordinates are virtually identical, and this test doesn't seem to be trying to establish which image gets picked, it's just bad luck that this regression test could fall into either. (arguably there could be some sort of periodic assert coordinates equal function for things like this...) The moment of inertia error is a little more tricky to pin down in a similar way, but I'd be suspicious that it is also a rounding error I got these tests to fail locally, and then they started passing locally, so it looks a bit twitchy... |
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.
barring the PBC precision fails, something I noticed is that for some reason group.center_of_mass()
doesn't seem to create the cache entry, which is confusing as the code should be going through this new cache path. .accumulate()
does seem to be creating the cache entry.
Thanks for looking into this @richardjgowers. I can also try to debug why the |
Changes made in this Pull Request:
Add a cache for creating the compound masks in a group. This speeds up the calculations of
accumulate
other methods that use_split_by_compound_indices
:Running a simple timing test with caching gives on my machine
397 µs ± 1.55 µs per loop (mean ± std. dev. of 7 runs, 1,000 loops each)
and with "disabled" cache
1.16 ms ± 11 µs per loop (mean ± std. dev. of 7 runs, 1,000 loops each)
PR Checklist
Developers certificate of origin
📚 Documentation preview 📚: https://mdanalysis--4612.org.readthedocs.build/en/4612/