Skip to content
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

Fixed NSGrid #3133

Merged
merged 6 commits into from
Mar 7, 2021
Merged

Fixed NSGrid #3133

merged 6 commits into from
Mar 7, 2021

Conversation

richardjgowers
Copy link
Member

Rewrote lib.nsgrid module

Fixes #2919 #2345 #2229

Changes made in this Pull Request:

PR Checklist

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

Rewrote lib.nsgrid module

Issues #2919 #2345 #2229
@pep8speaks
Copy link

pep8speaks commented Feb 28, 2021

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

Line 460:5: E124 closing bracket does not match visual indentation
Line 878:5: E124 closing bracket does not match visual indentation

Line 428:30: E127 continuation line over-indented for visual indent
Line 428:80: E501 line too long (84 > 79 characters)
Line 659:14: E261 at least two spaces before inline comment
Line 659:80: E501 line too long (95 > 79 characters)

Line 192:8: E714 test for object identity should be 'is not'
Line 202:16: E714 test for object identity should be 'is not'
Line 209:16: E714 test for object identity should be 'is not'

Line 55:1: E302 expected 2 blank lines, found 1
Line 59:80: E501 line too long (85 > 79 characters)
Line 60:80: E501 line too long (101 > 79 characters)
Line 101:80: E501 line too long (89 > 79 characters)
Line 307:80: E501 line too long (87 > 79 characters)

Line 135:80: E501 line too long (85 > 79 characters)

Comment last updated at 2021-03-07 02:51:00 UTC

@IAlibay IAlibay added this to the 1.0.2 milestone Mar 1, 2021
@zemanj
Copy link
Member

zemanj commented Mar 1, 2021

@richardjgowers does this PR supersede #2937?
Sorry for the long radio silence from my side, I'll review ASAP.

@richardjgowers
Copy link
Member Author

@zemanj yes. This is a rebase of that PR onto master instead of develop, so it gets released sooner. If you could review soon that would be great.

@IAlibay IAlibay mentioned this pull request Mar 7, 2021
3 tasks
@IAlibay
Copy link
Member

IAlibay commented Mar 7, 2021

@richardjgowers I went ahead and reverted the temporary removal of nsgrid from test_distances, hopefully that should fix the failures that were happening.

@codecov
Copy link

codecov bot commented Mar 7, 2021

Codecov Report

Merging #3133 (c52d614) into master (bc95e31) will decrease coverage by 0.36%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3133      +/-   ##
==========================================
- Coverage   90.90%   90.53%   -0.37%     
==========================================
  Files         149      150       +1     
  Lines       20694    20528     -166     
  Branches     3194     3188       -6     
==========================================
- Hits        18812    18586     -226     
- Misses       1248     1318      +70     
+ Partials      634      624      -10     
Impacted Files Coverage Δ
package/MDAnalysis/lib/distances.py 97.47% <100.00%> (+14.82%) ⬆️
package/MDAnalysis/version.py 100.00% <100.00%> (ø)
package/MDAnalysis/coordinates/chemfiles.py 28.48% <0.00%> (-62.00%) ⬇️
package/MDAnalysis/analysis/hole.py 71.78% <0.00%> (-0.83%) ⬇️
package/MDAnalysis/auxiliary/base.py 88.35% <0.00%> (-0.72%) ⬇️
package/MDAnalysis/core/universe.py 95.73% <0.00%> (-0.63%) ⬇️
package/MDAnalysis/coordinates/chain.py 86.82% <0.00%> (-0.62%) ⬇️
package/MDAnalysis/coordinates/TRZ.py 81.91% <0.00%> (-0.40%) ⬇️
package/MDAnalysis/coordinates/base.py 93.20% <0.00%> (-0.37%) ⬇️
package/MDAnalysis/coordinates/DMS.py 88.23% <0.00%> (-0.34%) ⬇️
... and 39 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 bc95e31...c52d614. Read the comment docs.

@IAlibay
Copy link
Member

IAlibay commented Mar 7, 2021

=.= I have no idea why it's now suddenly picking up numpy 1.20... we didn't have these issues up until now. Technically all the failures we are saying should get fixed by #3139, I guess the options here are a) pin CI temporarily, b) merge the code in #3139 into this branch (although it might end up making it really hard to review the changes made here).

Copy link
Member

@lilyminium lilyminium left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I shallowly skimmed the pyx file so have no real comments on that -- I'm going to approve this based on the tests directly addressing NSGrid issues.

@richardjgowers richardjgowers merged commit d9dcdcd into master Mar 7, 2021
@richardjgowers richardjgowers deleted the capped_distance_fix_1.0.2 branch March 7, 2021 19:15
@zemanj
Copy link
Member

zemanj commented Mar 20, 2021

@lilyminium It turns out that "shallowly skimming the pyx file" was not a great idea at all.
I know I should have reviewed the code already weeks ago but my free time is very limited these days.
Anyway, we need to be extremely careful with code that is of such fundamental importance for the package as this one, especially since this PR was for the master branch.

Even though the code is now much nicer than before, I discovered the following problems:

  • The function coordintoprimarycell() does not guarantee that atoms will really lie within the box and should be replaced by _triclinic_pbc() from calc_distances.h.
  • The test for issue FastNS yields wrong results on simple test case #2345 does not reproduce the respective system properly because it is stored as a pdb file (which lacks the necessary precision).
  • The code segfaults if box dimensions are large and the cutoff comparatively small.
  • It is relatively slow for low densities (too few atoms per cell).

Issues (and PR for the first two points) incoming.

@IAlibay
Copy link
Member

IAlibay commented Mar 21, 2021

Thanks for looking into this @zemanj!

Anyway, we need to be extremely careful with code that is of such fundamental importance for the package as this one, especially since this PR was for the master branch.

I agree. To the remaining @MDAnalysis/coredevs given that tomorrow is meant to be the code freeze for 1.1.0, I propose that we hold off on doing so until everything (at least non-performance related) is fixed for NSGrid.

@lilyminium
Copy link
Member

@lilyminium It turns out that "shallowly skimming the pyx file" was not a great idea at all.
I know I should have reviewed the code already weeks ago but my free time is very limited these days.
Anyway, we need to be extremely careful with code that is of such fundamental importance for the package as this one, especially since this PR was for the master branch.

Thanks a lot for checking the code and discovering those problems, @zemanj. I shouldn't have reviewed when pressed for time. Although to be honest, I doubt I would have discovered the issues anyway due to my unfamiliarity with this part of the code and the possible failure cases 😬

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants