Add check for too many grids in NSGrid #3441
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #3441 +/- ##
========================================
Coverage 93.75% 93.75%
========================================
Files 176 176
Lines 23163 23163
Branches 3297 3297
========================================
Hits 21717 21717
Misses 1395 1395
Partials 51 51
Continue to review full report at Codecov.
|
|
I'm a bit confused by the coverage output - it's not really possible that this change could have made such a delta. I'd suspect that some test might be failing now (and therefore not covering other things it would have), but the mdanalysis-CI seems to pass. |
It's just complaining because the majority of CI hasn't run yet, so the diff is off. Actions has this whole limit first time contributors thing and it's only recently that they introduced relaxed rules so we've not changed it back |
There was a problem hiding this comment.
Probably out of scope for PR, but we may also want to use Py_ssize_t for indexing:
https://stackoverflow.com/q/20987390/2942522
I probably need to follow that advice more often myself (using int is pretty common indeed though).
|
We noticed some improved -O3 autovectorisation with unsigned size_t over
int in distopia so worth doing IMO. Py_ssize_t is signed but possible worth
adhering too anyway esp if reccomended by Cython devs.
|
Good point. I wonder if we should still fail, since it's over a billion cells and very unlikely to be performant. Not sure where this team's preference lies between automatically adjusting parameters vs failing and having the user adjust themselves. I guess it makes sense for us to do it, given that it won't affect the algorithm accuracy to adjust the grid size upwards, but now there's the question of what to cap it at? My first thought is to increase the grid size so that there's at least an average of x particles per grid, but we might need to do some research on what a good limit is. |
There was a problem hiding this comment.
If we're up at ~1,000 cells a side, and even if we're searching for bonds (so r=1A), you've got a box with 100nm sides. This probably has about 100 million atoms in it? I'm not sure brute force methods are going to be more performant here.
I think at best this is a warning that the number of cells got truncated and that the devs should finally write some templates/fused types to handle the ultra large case.
|
That said, that all assumed heterogenous positions which might not be true. If a ratio of particles per cell fell below some heuristic threshold, then switching to a different algorithm (sparse cell grid thing) might well be a good idea. |
The example in the original issue is a counterexample here, where a box with a smaller number of atoms is inflated. The grid is set based off of the box size and cutoff, so you end up with more grids than particles to pack onto the grid. I was suggesting something like increasing the grid size to at least achieve some reasonable particle density. |
|
In the interest of moving this along, how about I implement the suggestion to cap the number of boxes, and we can revisit for optimizations. I'll see how this affects the comments regarding unit tests and apply whatever suggestions are still relevant. |
|
Yep sounds good 👍
…On Wed, Oct 20, 2021 at 15:53, Kevin Boyd ***@***.***> wrote:
In the interest of moving this along, how about I implement the suggestion
to cap the number of boxes, and we can revisit for optimizations. I'll see
how this affects the comments regarding unit tests and apply whatever
suggestions are still relevant.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#3441 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACGSGB3NRU74VU2V2A3YGMLUH3CWBANCNFSM5GD5BMJQ>
.
|
|
Hello @scal444! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
Comment last updated at 2021-11-09 15:09:32 UTC |
|
RE pep8 violations - if I run autopep8 it flags other problems in the same file. What's the project's stance on refactoring? In general, guess it's better to make a preliminary cleanup change, but here should I just add in the other style changes to the same PR? Or just change the affected lines? |
|
Thanks for asking — we tend to keep style changes only to the code that is currently being changed, i.e. the only ones that get flagged by the bot. It does mean that we have some code with a lot of violations, as you can see, but it does mean that we keep the git history a bit cleaner (it can be very handy to run git blame on a line of code and look up why it’s like that).
… On 24 Oct 2021, at 4:22 pm, Kevin Boyd ***@***.***> wrote:
RE pep8 violations - if I run autopep8 it flags other problems in the same file. What's the project's stance on refactoring? In general, guess it's better to make a preliminary cleanup change, but here should I just add in the other style changes to the same PR? Or just change the affected lines?
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub <#3441 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AHNMOXPC7NHRIDJHIRIOWMLUISIK5ANCNFSM5GD5BMJQ>.
|
|
Thanks for incorporating feedback @scal444! I will approve workflow run, its blocked because you are a first time contributor. |
The NSGrid cython code uses integer indexing, which is typically 32 bit. 3D grids with small cutoffs or giant box sizes can have grids > 1000 per dimension, which leads to integer overflow when trying to index the grids. Fixes MDAnalysis#3183
Rather than throwing an exception, the number of grids is capped, not affecting the algorithm output
|
The issue is almost literally a memory segfault, so… no? :)
…On Tue, Oct 26, 2021 at 17:25, Irfan Alibay ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In testsuite/MDAnalysisTests/lib/test_nsgrid.py
<#3441 (comment)>
:
> @@ -159,12 +163,16 @@ def test_nsgrid_distances(universe):
@pytest.mark.parametrize('box, results',
((None, [3, 13, 24]),
- (np.array([10., 10., 10., 90., 90., 90.]), [3, 13, 24, 39, 67]),
- (np.array([10., 10., 10., 60., 75., 90.]), [3, 13, 24, 39, 60, 79])))
+ (np.array([10000., 10000., 10000., 90., 90., 90.]),
+ [3, 13, 24]),
(I'm sure there's some env variable set on our CI matrix we can detect)
pretty sure actions sets $CI to True whenever it runs (see:
https://docs.github.com/en/actions/learn-github-actions/environment-variables
)
Is there a way we can reduce the memory cost but still replicate the
issue? I would prefer not have re-add psutil as a dependency if we can
avoid it.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#3441 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACGSGB3RI5NP44F6D4CUNZ3UI3I6PANCNFSM5GD5BMJQ>
.
|
We'll want to make this skip on more than just CI - we can't make an 8 GB memory test run on folk's laptops, it's going to be a nightmare for folks on lower end workstations and laptops. |
Alternatively - could we patch the max grid dim variable in the test to bring it down to something smaller? We'd still be testing the same error case, and it won't change the rest of the algorithm. Not sure how to override cython constants in python unit tests but there's probably something we could do. |
The constraint is going to be very baked in. Cython goes sideways into C++ then gets compiled, so the constraint is a constant somewhere in the object code. For the same reason, I'd rather not have it as a user variable. |
|
Does a DEF work here ? I’m not sure …
On Thu, 28 Oct 2021 at 8:30 pm, Richard Gowers ***@***.***> wrote:
We'll want to make this skip on more than just CI - we can't make an 8 GB
memory test run on folk's laptops, it's going to be a nightmare for folks
on lower end workstations and laptops.
Alternatively - could we patch the max grid dim variable in the test to
bring it down to something smaller? We'd still be testing the same error
case, and it won't change the rest of the algorithm. Not sure how to
override cython constants in python unit tests but there's probably
something we could do.
The constraint is going to be very baked in. Cython goes sideways into C++
then gets compiled, so the constraint is a constant somewhere in the object
code. For the same reason, I'd rather not have it as a user variable.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#3441 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AF3RHC7AH33EYCE25K3J6U3UJEQ4TANCNFSM5GD5BMJQ>
.
--
*Hugo MacDermott-Opeskin*
PhD Candidate, RSC ANU
Email: ***@***.***
***@***.***>
|
|
DEF is #define I think. You’d have to recompile the whole thing again with
the new constant.
On Thu, Oct 28, 2021 at 14:04, Hugo MacDermott-Opeskin <
***@***.***> wrote:
… Does a DEF work here ? I’m not sure …
On Thu, 28 Oct 2021 at 8:30 pm, Richard Gowers ***@***.***>
wrote:
> We'll want to make this skip on more than just CI - we can't make an 8 GB
> memory test run on folk's laptops, it's going to be a nightmare for folks
> on lower end workstations and laptops.
>
> Alternatively - could we patch the max grid dim variable in the test to
> bring it down to something smaller? We'd still be testing the same error
> case, and it won't change the rest of the algorithm. Not sure how to
> override cython constants in python unit tests but there's probably
> something we could do.
>
> The constraint is going to be very baked in. Cython goes sideways into
C++
> then gets compiled, so the constraint is a constant somewhere in the
object
> code. For the same reason, I'd rather not have it as a user variable.
>
> —
> You are receiving this because you commented.
> Reply to this email directly, view it on GitHub
> <
#3441 (comment)
>,
> or unsubscribe
> <
https://github.com/notifications/unsubscribe-auth/AF3RHC7AH33EYCE25K3J6U3UJEQ4TANCNFSM5GD5BMJQ
>
> .
>
--
*Hugo MacDermott-Opeskin*
PhD Candidate, RSC ANU
Email: ***@***.***
***@***.***>
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#3441 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACGSGB3DXPKLSSF5FIEE3XDUJE34JANCNFSM5GD5BMJQ>
.
|
|
I was thinking something like: DEF test_ns_grid_max = 1 # set on compile, only to run on special occasions, like Christmas
IF test_ns_grid_max
DEF MAX_DIM=reasonable_value_to_test
ELSE
DEF MAX_DIM= 1290but Im probably missing something / its super ugly / disabling on CI is much cleaner. |
Yeah this would work, but you'd be reinstalling the entire package for a single test. I think it's probably fine not to test for this (unless someone is actively tinkering with the algorithm..). |
I think we're overthinking this. As @richardjgowers first pointed out, let's just guard the test with an env variable, set it in our macOS runners (which for now have >10 GB ram), and document it somewhere - job done. |
|
Good point @IAlibay, sorry for muddying the waters, much better to just block test. |
Enable test in macos runners which have sufficient memory
|
Test is now guarded by an env variable |
There was a problem hiding this comment.
Just the one thing on my end. Aside from @richardjgowers' comment, everything else looks good. Thanks @scal444 !
Used parentheses for string concatenation and a bool check converter
|
@scal444 thanks for fixing this and congrats on getting your first PR on the board! |
The NSGrid cython code uses integer indexing, which is typically
32 bit. 3D grids with small cutoffs or giant box sizes can have
grids > 1000 per dimension, which leads to integer overflow when
trying to index the grids.
Fixes #3183
Changes made in this Pull Request:
PR Checklist