Fixes AtomGroup.center when using compounds + unwrapping #2992

mnmelo · 2020-10-17T02:57:14Z

In AtomGroup.center, coordinates are now properly sorted after unwrapping, in tandem with compound_indices and weights. Additionally, sorting is now only even done if the compound_indices are not already monotonically increasing. Monotonicity is a rather cheap check for the most frequent use case, compared to the expensive sorting that's only really needed for the rarer cases of unordered/noncontiguous compounds.

PR Checklist

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

… with unwrapping (closes #2984).

pep8speaks · 2020-10-17T02:57:18Z

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

In the file testsuite/MDAnalysisTests/core/test_atomgroup.py:

Line 1024:31: E201 whitespace after '['
Line 1027:31: E201 whitespace after '['
Line 1067:80: E501 line too long (84 > 79 characters)
Line 1068:80: E501 line too long (133 > 79 characters)
Line 1069:80: E501 line too long (129 > 79 characters)
Line 1070:80: E501 line too long (132 > 79 characters)
Line 1071:80: E501 line too long (127 > 79 characters)
Line 1303:39: E127 continuation line over-indented for visual indent
Line 1315:39: E127 continuation line over-indented for visual indent
Line 1344:39: E127 continuation line over-indented for visual indent
Line 1358:39: E127 continuation line over-indented for visual indent

codecov · 2020-10-17T05:02:47Z

Codecov Report

Merging #2992 into develop will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff            @@
##           develop    #2992   +/-   ##
========================================
  Coverage    93.05%   93.05%           
========================================
  Files          186      186           
  Lines        24609    24611    +2     
  Branches      3187     3188    +1     
========================================
+ Hits         22900    22902    +2     
  Misses        1661     1661           
  Partials        48       48

Impacted Files	Coverage Δ
package/MDAnalysis/core/groups.py	`98.58% <100.00%> (+<0.01%)`	⬆️

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 26880f0...2a91314. Read the comment docs.

jbarnoud

There are a couple of changes of indentation for split lines that are make the linter complain. Other than that there only is a small change on one test that I think would improve the PR.

jbarnoud · 2020-10-17T12:38:24Z

testsuite/MDAnalysisTests/core/test_atomgroup.py

@@ -1058,6 +1064,12 @@ def test_UnWrapFlag_residues(self, ag, ref_Unwrap_residues):
        assert_almost_equal(ag.moment_of_inertia(unwrap=True, compound='residues'), ref_Unwrap_residues['MOI'], self.prec)
        assert_almost_equal(ag.asphericity(unwrap=True, compound='residues'), ref_Unwrap_residues['Asph'], self.prec)

+    def test_UnWrapFlag_residues_unordered(self, unordered_ag, ref_Unwrap_residues):


This could be parametrized so each AG method is tested separately.

You suggest also parameterizing the method? Like this?

@pytest.mark.parametrize('method_name', ('center_of_geometry', 'center_of_mass', 'moment_of_inertia', 'asphericity')) def test_UnWrapFlag_residues_unordered(self, unordered_ag, ref_Unwrap_residues, method_name): method = getattr(unordered_ag, method_name) # requires that the keys of ref_Unwrap_residues are changed to match the method names assert_almost_equal(method(unwrap=True, compound='residues'), ref_Unwrap_residues[method_name], self.prec)

I'm all for it, but several more tests are begging for the same cleanup. It seems a targeted refactoring effort would make more sense. If you think that at least new tests should be parameterized as above, I'm cool with having it thus.

jbarnoud · 2020-10-17T12:40:02Z

testsuite/MDAnalysisTests/core/test_atomgroup.py

@@ -1058,6 +1064,12 @@ def test_UnWrapFlag_residues(self, ag, ref_Unwrap_residues):
        assert_almost_equal(ag.moment_of_inertia(unwrap=True, compound='residues'), ref_Unwrap_residues['MOI'], self.prec)
        assert_almost_equal(ag.asphericity(unwrap=True, compound='residues'), ref_Unwrap_residues['Asph'], self.prec)

+    def test_UnWrapFlag_residues_unordered(self, unordered_ag, ref_Unwrap_residues):
+        assert_almost_equal(unordered_ag.center_of_geometry(unwrap=True, compound='residues'), ref_Unwrap_residues['COG'], self.prec)


These lines could be split.

I really just kept them long to conform to how all other such tests are. I can split (shall I also split the analogous ones in neighboring tests?)

mnmelo · 2020-10-17T19:30:21Z

In the end, most of the non-parameterization or lint in the tests is to conform to the existing standard of neighboring tests. I'll gladly introduce these tests as exceptions and examples to follow from now on, but I think a refactor should be its own PR.

jbarnoud

Refactoring the tests is out of scope for this PR and I agree there is some value in keeping internal consistency. I openned an issue about the cleaning of the tests in that file: #2995.

… with unwrapping (PR #2992) fixes #2984 (cherry picked from commit beb5232)

… with unwrapping (PR MDAnalysis#2992) fixes MDAnalysis#2984 (cherry picked from commit beb5232)

… with (MDAnalysis#2992) unwrapping (closes MDAnalysis#2984).

Fixed AtomGroup.center's behavior when using compounds in conjugation…

2a91314

… with unwrapping (closes #2984).

mnmelo added defect PBC Priority-High labels Oct 17, 2020

jbarnoud requested changes Oct 17, 2020

View reviewed changes

jbarnoud mentioned this pull request Oct 18, 2020

testsuite/MDAnalysisTests/core/test_atomgroup.py would benefit from some cleaning #2995

Closed

jbarnoud approved these changes Oct 18, 2020

View reviewed changes

jbarnoud merged commit beb5232 into develop Oct 18, 2020

jbarnoud deleted the issue-2984-center_components_unwrapping branch October 18, 2020 09:35

mnmelo mentioned this pull request Oct 20, 2020

Use of compounds involves excessive sorting and fancy-indexing of AtomGroups and compound indices. #3000

Closed

orbeckst pushed a commit that referenced this pull request Oct 23, 2020

Fixed AtomGroup.center's behavior when using compounds in conjugation…

6580c80

… with unwrapping (PR #2992) fixes #2984 (cherry picked from commit beb5232)

orbeckst mentioned this pull request Oct 23, 2020

Fixed AtomGroup.center's behavior when using compounds in conjugation with unwrapping #3010

Merged

orbeckst pushed a commit that referenced this pull request Oct 24, 2020

Fixed AtomGroup.center's behavior when using compounds in conjugation…

49cb0a8

… with unwrapping (PR #2992) fixes #2984 (cherry picked from commit beb5232)

mnmelo mentioned this pull request Oct 25, 2020

Cleanup and better parametrization of test_atomgroup.py #3012

Merged

4 tasks

lilyminium pushed a commit to lilyminium/mdanalysis that referenced this pull request Dec 9, 2020

Fixed AtomGroup.center's behavior when using compounds in conjugation…

b37f518

… with unwrapping (PR MDAnalysis#2992) fixes MDAnalysis#2984 (cherry picked from commit beb5232)

PicoCentauri pushed a commit to PicoCentauri/mdanalysis that referenced this pull request Mar 30, 2021

Fixed AtomGroup.center's behavior when using compounds in conjugation…

675ea0c

… with (MDAnalysis#2992) unwrapping (closes MDAnalysis#2984).

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes AtomGroup.center when using compounds + unwrapping #2992

Fixes AtomGroup.center when using compounds + unwrapping #2992

mnmelo commented Oct 17, 2020

pep8speaks commented Oct 17, 2020

codecov bot commented Oct 17, 2020 •

edited

Loading

jbarnoud left a comment

jbarnoud Oct 17, 2020

mnmelo Oct 17, 2020

jbarnoud Oct 17, 2020

mnmelo Oct 17, 2020 •

edited

Loading

mnmelo commented Oct 17, 2020

jbarnoud left a comment

Fixes AtomGroup.center when using compounds + unwrapping #2992

Fixes AtomGroup.center when using compounds + unwrapping #2992

Conversation

mnmelo commented Oct 17, 2020

PR Checklist

pep8speaks commented Oct 17, 2020

codecov bot commented Oct 17, 2020 • edited Loading

Codecov Report

jbarnoud left a comment

Choose a reason for hiding this comment

jbarnoud Oct 17, 2020

Choose a reason for hiding this comment

mnmelo Oct 17, 2020

Choose a reason for hiding this comment

jbarnoud Oct 17, 2020

Choose a reason for hiding this comment

mnmelo Oct 17, 2020 • edited Loading

Choose a reason for hiding this comment

mnmelo commented Oct 17, 2020

jbarnoud left a comment

Choose a reason for hiding this comment

codecov bot commented Oct 17, 2020 •

edited

Loading

mnmelo Oct 17, 2020 •

edited

Loading