Temporarily disable nsgrid (Issue #2930) by IAlibay · Pull Request #3004 · MDAnalysis/mdanalysis

IAlibay · 2020-10-23T01:32:35Z

Fixes #2930

Changes made in this Pull Request:

Temporarily replaces nsgrid with pkdtree
Adds note on the use of nsgrid

PR Checklist

Tests?
Docs?
CHANGELOG updated?
Issue raised/referenced?

pep8speaks · 2020-10-23T01:32:40Z

Hello @IAlibay! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

In the file testsuite/MDAnalysisTests/lib/test_distances.py:

Line 99:37: E261 at least two spaces before inline comment
Line 223:63: E261 at least two spaces before inline comment
Line 223:80: E501 line too long (86 > 79 characters)
Line 238:58: E261 at least two spaces before inline comment
Line 238:80: E501 line too long (81 > 79 characters)
Line 240:61: E261 at least two spaces before inline comment
Line 240:80: E501 line too long (84 > 79 characters)
Line 994:69: E261 at least two spaces before inline comment
Line 994:80: E501 line too long (86 > 79 characters)
Line 1003:69: E261 at least two spaces before inline comment
Line 1003:80: E501 line too long (86 > 79 characters)
Line 1104:69: E261 at least two spaces before inline comment
Line 1104:80: E501 line too long (86 > 79 characters)
Line 1120:69: E261 at least two spaces before inline comment
Line 1120:80: E501 line too long (86 > 79 characters)
Line 1237:69: E261 at least two spaces before inline comment
Line 1237:80: E501 line too long (86 > 79 characters)
Line 1260:69: E261 at least two spaces before inline comment
Line 1260:80: E501 line too long (86 > 79 characters)

Comment last updated at 2020-12-22 13:01:12 UTC

IAlibay · 2020-10-23T01:36:24Z

package/MDAnalysis/lib/nsgrid.pyx


 .. versionadded:: 0.19.0

+Important Note


I have only left a note, would a warning be useful too? (not really sure how you do warnings in a cython file).

Do you mean import warnings? I think that should work.

Can you also add a warning in the markup

.. warning:: The current nsgrid code can return incorrect values. Do not use for production use. See issue `#2930<https://github.com/MDAnalysis/mdanalysis/issues/2930>`_ for further context.

(I hope that's the right syntax for inline reST links).

codecov · 2020-10-23T02:48:00Z

Codecov Report

Merging #3004 (fc5cc2c) into master (ebd9c02) will increase coverage by 0.64%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #3004      +/-   ##
==========================================
+ Coverage   91.36%   92.00%   +0.64%     
==========================================
  Files         161      166       +5     
  Lines       22224    22734     +510     
  Branches     3194     3194              
==========================================
+ Hits        20304    20916     +612     
- Misses       1290     1726     +436     
+ Partials      630       92     -538

Impacted Files	Coverage Δ
package/MDAnalysis/lib/distances.py	`84.85% <100.00%> (-12.62%)`	⬇️
package/MDAnalysis/lib/nsgrid.pyx	`91.45% <100.00%> (+0.15%)`	⬆️
package/MDAnalysis/topology/tpr/obj.py	`96.82% <0.00%> (-3.18%)`	⬇️
...onality_reduction/DimensionalityReductionMethod.py	`97.14% <0.00%> (-2.86%)`	⬇️
package/MDAnalysis/due.py	`56.09% <0.00%> (ø)`
package/MDAnalysis/analysis/legacy/x3dna.py	`0.00% <0.00%> (ø)`
package/MDAnalysis/tests/__init__.py	`100.00% <0.00%> (ø)`
package/MDAnalysis/tests/datafiles.py	`31.25% <0.00%> (ø)`
package/MDAnalysis/analysis/legacy/__init__.py	`0.00% <0.00%> (ø)`
... and 87 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ebd9c02...fc5cc2c. Read the comment docs.

orbeckst

Thanks @IAlibay ! Looking good to me; I don't know if there are any more places where it might show up, though.

I assume the actual tests for the code are still in place?

I have minor comments/suggestions for docs – please see inline.

package/CHANGELOG

orbeckst · 2020-10-23T19:26:06Z

package/MDAnalysis/lib/nsgrid.pyx


 .. versionadded:: 0.19.0

+Important Note


Do you mean import warnings? I think that should work.

Can you also add a warning in the markup

.. warning:: The current nsgrid code can return incorrect values. Do not use for production use. See issue `#2930<https://github.com/MDAnalysis/mdanalysis/issues/2930>`_ for further context.

(I hope that's the right syntax for inline reST links).

IAlibay · 2020-10-23T21:18:40Z

I don't know if there are any more places where it might show up, though.

So I can't seem to find any more cases of FastNS being imported (just doing a recursive grep for files in package

Outside of lib.__init__, the only other instance of nsgrid is in setup.py's extensions (from what I gather it's just the cythonize stuff).

I assume the actual tests for the code are still in place?

So the tests that have been affected here are those that invoke capped_distance or self_capped_distance, so it lowers coverage of nsgrid a little bit, but test_nsgrid is still there.

orbeckst · 2020-10-23T22:50:03Z

@IAlibay please merge at your leisure (unless anyone else finds something, of course)

IAlibay · 2020-10-23T22:51:58Z

@IAlibay please merge at your leisure (unless anyone else finds something, of course)

Will do, just adding those warnings and this should be good :)

orbeckst · 2020-10-24T04:58:12Z

Apart from my own stupidity (see #3011 ...) this is the last real thing to be completed for 1.0.1 to ship.

IAlibay · 2020-10-24T18:06:46Z

Ok, changed the docs to have proper .. warning entries, and added a userwarning on FastNS.

pinging @MDAnalysis/coredevs can someone give this a quick re-review so we can merge with the aim of prepping the 1.0.1 release tomorrow (@richardjgowers nicely suggested walking those interested to learn through it).

orbeckst · 2020-10-25T04:19:11Z

The tests fail because the fastns warning is not always raised. Perhaps we have to ensure that it is always raised and never muted?

IAlibay · 2020-10-25T16:54:19Z

The tests fail because the fastns warning is not always raised. Perhaps we have to ensure that it is always raised and never muted?

I honestly have no idea why this isn't working. It works fine if I build locally on my end 😕 The fact that azure builds and the other CIs don't probably mean there's an option we are/aren't turning on somewhere that disables this warning but I can't seem to work out what.

orbeckst · 2020-10-26T23:52:57Z

If we make it something that's not a UserWarning such as RuntimeWarning???

Alternatively, can you enforce raising warnings in your test? Some of the text in https://docs.python.org/3.8/library/warnings.html#testing-warnings and above says that any of the fancy temporarily changing warning behavior is not well defined in multi-threaded applications. I think parallel pytest uses different processes so that should be ok. There's also a note

One thing to be aware of is that if a warning has already been raised because of a once/default rule, then no matter what filters are set the warning will not be seen again unless the warnings registry related to the warning has been cleared.

which suggests that we might have to globally set our warnings filter at the beginning of the tests to ensure that we always see our warnings.

This reverts commit 5478483.

IAlibay · 2020-10-28T19:11:54Z

Now this is quite suspicious, d4499f5 should have caused a ton of CI tests to fail (as per the azure tests( because that RuntimeError should have been encountered on class construction (unless I typo-ed somewhere).

The fact that only the nsgrid test for the RuntimeWarning failed is very puzzling.

Is it possible that somehow CI isn't recompiling the cython files? Pinging @richardjgowers and @tylerjereddy who might know better about the peculiarity of our Travis setup & cython compiling.

orbeckst · 2020-10-30T20:06:07Z

Following https://docs.python.org/3/library/warnings.html#overriding-the-default-filter, perhaps we should add an "always" at the start of our tests in testsuite/MDAnalysisTests/__init__.py

import sys

if not sys.warnoptions:
    import os, warnings
    warnings.simplefilter("always") # Change the filter in this process
    os.environ["PYTHONWARNINGS"] = "always" # Also affect subprocesses

(I would have assumed that pytest is already doing something like this but who knows?)

This should ensure that warnings are never counted as "have already occurred" and hopefully get around the problem

One thing to be aware of is that if a warning has already been raised because of a once/default rule, then no matter what filters are set the warning will not be seen again unless the warnings registry related to the warning has been cleared.

One thing to check would be if pytest.warns() can be made to reset this registry ~~(with warnings.resetwarnings()~~ to ensure that a warning is always shown.

~~Or use warnings.resetwarnings() in your own test to check???~~ EDIT: I think I was mistaken in believing that this resets the count of warnings... needs more research.

IAlibay · 2020-10-30T20:13:37Z

@orbeckst the problem here isn't that the warning isn't being emitted, it's that the new cython code isn't being compiled to c to begin with.

Looking at differences between azure (which works) and the other two CI methods, I suspect the problem is how we call setup.py. @richardjgowers' suggestion here was to try just check the new c files in, however I'd prefer fix CI to begin with (if possible).

orbeckst · 2020-10-30T22:40:32Z

If we are not rebuilding the C files then what are we are actually testing??

~~How did you find out that the files are not being rebuild?~~ EDIT: saw it above

orbeckst · 2020-10-30T22:57:49Z

The Travis output only contains Running setup.py develop for MDAnalysis for the step that we're interested in. Do you want to try and replace

mdanalysis/.travis.yml

Line 31 in 2ef90f2

    
           - BUILD_CMD="pip install -e package/ && (cd testsuite/ && python setup.py build)"

with

BUILD_CMD="(cd package && python setup.py install) && (cd testsuite/ && python setup.py build)"

to have the same across our CI providers? (I have no idea why we used a developer install there anyway...)

…into disable-nsgrid-1.0.1

IAlibay · 2020-11-02T17:37:59Z

Apologies, I've not had time to really spend on this. I'm going to open a separate test PR targeting develop just to see if I can isolate the cause of all this...

IAlibay · 2020-11-09T21:16:56Z

Ok, so we know that it works on #3024, so it's master specific, now to dig into why..

IAlibay · 2020-11-15T23:03:23Z

So I've spent a fair amount on this and I'm still baffled that what's in master that causes this behaviour. Given that we really should have pushed 1.0.1 a month ago, I'm going to ping @MDAnalysis/coredevs here. If anyone can come up with a quick solution this would be great.

The only option I can think of here is to push the new c files generated by cython, but I'm not sure if that's such a great idea long term (especially if we keep working on master towards future 1.0.x releases).

orbeckst · 2020-11-16T16:10:03Z

Thank you for all the hours you put into trying to figure it out!!!

Did you confirm locally that it works as expected? If so, in order to get 1.0.1 out I suggest you make the corresponding text XFAIL or XPASS (?) in 1.0.1, add a comment describing the problem and reference the discussion here. Let's move forward.

IAlibay · 2020-11-19T14:18:39Z

So finally yielding on fixing CI and following @richardjgowers' wisdom does the trick here.

It seems like we'll have to push re-generated .c/.cpp files on master for now.

Maybe the option is to create a separate develop-1.x branch for future releases (if needed), where we can remove all these files like we do on develop?

I've reverted all the CI yml changes for now since they had no impact.

@orbeckst does this work for you? If so we can merge & look to ship 1.0.1 soon.

orbeckst · 2020-11-30T21:05:40Z

Sorry, behind on things....

Let's move forward with the way you suggest. Using master for 1.x is dirty anyway, better be explicit. 🚀

lilyminium · 2020-12-07T23:52:50Z

Might the failure to recreate .c/pp files change now with GH actions?

IAlibay · 2020-12-22T13:02:48Z

Ok, this should be good to merge?

orbeckst · 2020-12-22T20:07:56Z

The sooner the better! 🚢

lilyminium

LGTM, thank you @IAlibay for pushing this through! 💫

Temporarily disable nsgrid (Issue MDAnalysis#2930)

ce11aeb

update changelog

a5020a2

IAlibay commented Oct 23, 2020

View reviewed changes

Add changelog details and fix typos

ffc0a72

orbeckst approved these changes Oct 23, 2020

View reviewed changes

orbeckst assigned IAlibay Oct 23, 2020

orbeckst mentioned this pull request Oct 24, 2020

Wrong results from capped_distance() with "nsgrid" search and atom in the center of the box #2919

Closed

Fixes docs warnings and adds new UserWarning

591faaa

IAlibay added 2 commits October 25, 2020 15:10

force warning

9d38120

try switching the test to the top

5478483

IAlibay added 3 commits October 27, 2020 13:50

Changes nsgrid warning to RuntimeWarning

0757943

Testing with an error

d4499f5

Revert last 3 commits

d108230

This reverts commit 5478483.

IAlibay added 2 commits October 30, 2020 03:33

change travis setup install behaviour

a80c7c7

fix testsuite install

d293ec5

IAlibay and others added 2 commits October 31, 2020 17:13

Merge branch 'master' into disable-nsgrid-1.0.1

541fcfc

Merge branch 'disable-nsgrid-1.0.1' of github.com:IAlibay/mdanalysis …

fdcac60

…into disable-nsgrid-1.0.1

IAlibay mentioned this pull request Nov 9, 2020

Adds nsgrid usage warning #3024

Closed

4 tasks

IAlibay added 4 commits November 10, 2020 12:02

switch appveyor isntall

3f72093

trying something out

dfacae4

test; check setup variables

adaa599

normalise version number

9442032

IAlibay added 2 commits November 19, 2020 12:26

Adds nsgrid cpp changes

1136352

Revert CI testing changes

378d6d5

lilyminium mentioned this pull request Dec 8, 2020

Port GH actions and other fixes to master #3061

Closed

4 tasks

Merge branch 'master' into disable-nsgrid-1.0.1

fc5cc2c

IAlibay requested a review from lilyminium December 22, 2020 13:02

lilyminium approved these changes Dec 22, 2020

View reviewed changes

lilyminium merged commit 6e2d8e7 into MDAnalysis:master Dec 22, 2020

lilyminium mentioned this pull request Dec 23, 2020

disable nsgrid neighbor search #2930

Closed

yuxuanzhuang mentioned this pull request Jul 29, 2021

On loading the same XTC offset in parallel #3375

Merged

4 tasks

fiona-naughton added defect Component-lib labels Sep 26, 2023

Conversation

IAlibay commented Oct 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Checklist

Uh oh!

pep8speaks commented Oct 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Comment last updated at 2020-12-22 13:01:12 UTC

Uh oh!

IAlibay Oct 23, 2020

Choose a reason for hiding this comment

Uh oh!

orbeckst Oct 23, 2020

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Oct 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

orbeckst left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

orbeckst Oct 23, 2020

Choose a reason for hiding this comment

Uh oh!

IAlibay commented Oct 23, 2020

Uh oh!

orbeckst commented Oct 23, 2020

Uh oh!

IAlibay commented Oct 23, 2020

Uh oh!

orbeckst commented Oct 24, 2020

Uh oh!

IAlibay commented Oct 24, 2020

Uh oh!

orbeckst commented Oct 25, 2020

Uh oh!

IAlibay commented Oct 25, 2020

Uh oh!

orbeckst commented Oct 26, 2020

Uh oh!

IAlibay commented Oct 28, 2020

Uh oh!

orbeckst commented Oct 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

IAlibay commented Oct 30, 2020

Uh oh!

orbeckst commented Oct 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

orbeckst commented Oct 30, 2020

Uh oh!

IAlibay commented Nov 2, 2020

Uh oh!

IAlibay commented Nov 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

IAlibay commented Nov 15, 2020

Uh oh!

orbeckst commented Nov 16, 2020

Uh oh!

IAlibay commented Nov 19, 2020

Uh oh!

orbeckst commented Nov 30, 2020

Uh oh!

lilyminium commented Dec 7, 2020

Uh oh!

IAlibay commented Dec 22, 2020

Uh oh!

orbeckst commented Dec 22, 2020

Uh oh!

lilyminium left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

IAlibay commented Oct 23, 2020 •

edited

Loading

pep8speaks commented Oct 23, 2020 •

edited

Loading

codecov bot commented Oct 23, 2020 •

edited

Loading

orbeckst commented Oct 30, 2020 •

edited

Loading

orbeckst commented Oct 30, 2020 •

edited

Loading

IAlibay commented Nov 9, 2020 •

edited

Loading

lilyminium left a comment •

edited

Loading