renamed 'pbc' kwarg to 'wrap' for Group methods by richardjgowers · Pull Request #2857 · MDAnalysis/mdanalysis

richardjgowers · 2020-07-19T13:34:22Z

Related to issue #1760 in particular @orbeckst here

Changes made in this Pull Request:

pbc kwarg changed to wrap for Group methods

The pbc kwarg for center_of_mass has been confusing people for a while, it's not clear if this wraps (ie forcing atoms to inside the unit cell) or unwrap (ie makes bonds whole, possibly putting atoms outside the unit cell).

The pbc option will still work for now but raise a deprecation warning

PR Checklist

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

pep8speaks · 2020-07-19T13:34:34Z

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

In the file package/MDAnalysis/core/groups.py:

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

In the file package/MDAnalysis/core/topologyattrs.py:

Line 1578:80: E501 line too long (80 > 79 characters)
Line 1581:80: E501 line too long (80 > 79 characters)
Line 1591:80: E501 line too long (84 > 79 characters)
Line 1596:80: E501 line too long (102 > 79 characters)
Line 1601:80: E501 line too long (101 > 79 characters)
Line 1602:80: E501 line too long (115 > 79 characters)
Line 1604:80: E501 line too long (101 > 79 characters)
Line 1608:80: E501 line too long (93 > 79 characters)

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

Line 1555:80: E501 line too long (89 > 79 characters)
Line 1567:80: E501 line too long (87 > 79 characters)

Comment last updated at 2022-03-05 16:25:15 UTC

richardjgowers · 2020-07-19T13:54:23Z

package/MDAnalysis/core/groups.py

+    """Raises ValueError when both 'wrap' and 'unwrap' are set to True"""
    @functools.wraps(function)
    def wrapped(group, *args, **kwargs):
-        if kwargs.get('compound') == 'group':


Here we used to allow pbc and unwrap if compound wasn't group, this slipped through review here: https://github.com/MDAnalysis/mdanalysis/pull/2275/files#r296183096

For sanity's sake, I'm saying that if you want a combination of wrapping/unwrapping you need to do multiple calls

richardjgowers · 2020-07-19T13:55:26Z

I'm also not sure what we should target for the pbc deprecation. It's not overly urgent we just need to nudge people away from using center_of_mass(pbc=True) but it's not going to break anything to leave the undocumented option

codecov · 2020-07-19T16:32:10Z

Codecov Report

Merging #2857 (bf4bfd7) into develop (db0c3d3) will increase coverage by 0.05%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop    #2857      +/-   ##
===========================================
+ Coverage    94.06%   94.11%   +0.05%     
===========================================
  Files          176      190      +14     
  Lines        23294    24656    +1362     
  Branches      3305     3309       +4     
===========================================
+ Hits         21911    23206    +1295     
- Misses        1337     1404      +67     
  Partials        46       46

Impacted Files	Coverage Δ
package/MDAnalysis/transformations/rotate.py	`100.00% <ø> (ø)`
package/MDAnalysis/transformations/translate.py	`95.74% <ø> (ø)`
package/MDAnalysis/core/groups.py	`98.58% <100.00%> (+0.01%)`	⬆️
package/MDAnalysis/core/topologyattrs.py	`96.63% <100.00%> (+0.01%)`	⬆️
package/MDAnalysis/converters/OpenMM.py	`97.22% <0.00%> (-0.12%)`	⬇️
package/MDAnalysis/units.py	`100.00% <0.00%> (ø)`
package/MDAnalysis/lib/c_distances.pyx	`100.00% <0.00%> (ø)`
package/MDAnalysis/converters/OpenMMParser.py	`100.00% <0.00%> (ø)`
package/MDAnalysis/lib/c_distances_openmp.pyx	`100.00% <0.00%> (ø)`
MDAnalysisTests/lib/__init__.py	`100.00% <0.00%> (ø)`
... and 14 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 db0c3d3...bf4bfd7. Read the comment docs.

jbarnoud · 2020-07-19T17:26:39Z

Just throwing #2299 and #2305 here for reference.

jbarnoud · 2020-07-19T17:34:18Z

package/MDAnalysis/core/groups.py

+    @functools.wraps(function)
+    def wrapped(group, *args, **kwargs):
+        if kwargs.get('pbc', None) is not None:
+            warnings.warn("Use 'wrap' kwarg not 'pbc'", DeprecationWarning)


The tone sounds a bit harsh, maybe.

orbeckst · 2020-07-21T06:13:22Z

If you split the PR into two:

deprecations (against master), to go into 1.0.1
this PR where you don't bother with deprecations

richardjgowers · 2020-07-21T07:09:02Z

@orbeckst the deprecations are in this PR, pbc=True still works but issues a warning. This doesn't need backporting.

orbeckst · 2020-07-21T07:45:52Z

I thought you wanted to get rid of pbc=True for 2.0. You can if you put the deprecations into 1.0.1.

Otherwise the next time you can get rid of them is 3.0.

orbeckst

Could you please make this a PR against 1.0 (master) to go into 1.0.1?

Then make a second PR against develop where you remove the deprecated functionality. I'd rather remove stuff for 2.0 than let it sit until 3.0. We have a good opportunity for proper cleanup here, and we're already spending user-goodwill so I'd make it count.

IAlibay · 2021-04-06T17:29:07Z

What's the status of this? I'm making a finalised list of the 2.0 milestones. I've included it for now.

orbeckst · 2021-04-06T18:05:46Z

I think my review comment still stands: if anything is removed in 2.0, it needs to be deprecated in 1.x

orbeckst · 2021-04-27T18:19:40Z

My opinion remains that

the pbc kwarg for should be deprecated in 1.1 (and we allow wrap as a synonym as in this PR)
we replace pbc with wrap in 2.0

Alternatively, we go ahead with the PR as it stands and allow wrap and pbc as synonyms for 2.x.

Given that @richardjgowers and I seem to be of different opinion how to move forward, it would be useful to have some other devs offer opinions.

IAlibay · 2021-04-27T18:32:00Z

My opinion remains that

the pbc kwarg for should be deprecated in 1.1 (and we allow wrap as a synonym as in this PR)

we replace pbc with wrap in 2.0

Alternatively, we go ahead with the PR as it stands and allow wrap and pbc as synonyms for 2.x.

Given that @richardjgowers and I seem to be of different opinion how to move forward, it would be useful to have some other devs offer opinions.

Whilst I agree that deprecating in 1.1 would be cleaner. I think at this point we just need to complete #3242 and cut a release. Probably going with synonyms and then deprecating so that we can remove in 3.0 would be our best option.

orbeckst

I am swayed by @IAlibay, let's deprecate pbc for removal in 3.0.

documentation
please also fix the benchmarks in benchmarks/benchmarks/ag_methods.py

package/MDAnalysis/core/groups.py

package/MDAnalysis/core/topologyattrs.py

orbeckst · 2021-05-11T06:38:06Z

@IAlibay this one was not on 2.0.0 milestone. It does not have to be because we'll deprecate for 3.0 but because unwrapping is something that we might want for the workshop (?), I thought I mention it.

IAlibay · 2021-05-11T11:51:57Z

@IAlibay this one was not on 2.0.0 milestone. It does not have to be because we'll deprecate for 3.0 but because unwrapping is something that we might want for the workshop (?), I thought I mention it.

Yeah that's why I originally added it to the 2.0 milestone last month. I don't mind either way, we just need to have enough developer time to get it done. How much work is left to deal with here?

IAlibay · 2021-08-17T18:02:53Z

@orbeckst are we good moving this to the 2.1 milestone?

orbeckst · 2021-08-17T22:44:49Z

sure, move it

orbeckst · 2021-10-28T18:37:38Z

Going through the comments, there doesn't seem to be an enormous amount of work to do, except that all the conflicts need to be resolved.

jbarnoud · 2022-02-04T15:31:08Z

@richardjgowers do you want to give the final touches on that PR? If you don't have time, I should be able to take care of it next week.

issue #1760

Rename a missed variable and remove calls that use both wrap and unwrap. This commit does not pass the tests because some of them rely on both wrap and unwrap in the same call. This will be fixed in a subsequent commit.

jbarnoud · 2022-02-15T12:30:38Z

I rebased the changes and fixed the conflicts. The tests are broken, though, because some tests have been introduced that rely on both unwrap and warp being used on the same call. I need to fix that. Some other tests may need fixing.

TODO:

fix broken tests
change version in the deprecation notices
mention version 3.0 in the deprecation warning

topologyobjects.Bond.length still has a pdb argument. This commit reflects this.

richardjgowers · 2022-02-16T10:49:34Z

@jbarnoud yes there are some things which have both unwrap and wrap. I think the idea was to first unwrap to calculate e.g. a CoM, then wrap these results back inside. I think this is extremely confusing as it's not clear which of wrap/unwrap is first see #2857 (comment)

jbarnoud · 2022-02-16T10:57:46Z

@richardjgowers Indeed. Removing ambiguity is a good thing. My main issue with the broken tests is that it is not clear if they are meant to test the wrap and the unwrap.

jbarnoud · 2022-03-03T10:03:57Z

@orbeckst Assuming the tests pass, it should be good to go. Since there is a deprecation, it would be good to have it in 2.1.0 that @IAlibay hopes to push soon.

Testing deprecated arguments lead to false positives in pylint. This commit ignores the specific warning on the problematic lines.

orbeckst

I had a quick look and as far as I can tell there are still a few minor documentation issues left.
EDIT: see my earlier comments.
EDIT 2: Some that say "out of date" are still valid, they just had some other changes nearby. I resolved everything that was clearly done. Please look at anything that's not been resolved and then please resolve it yourself — either because you fixed it or because you consider it out of scope or already done. Thanks.

package/MDAnalysis/core/topologyattrs.py

orbeckst · 2022-03-05T00:48:29Z

I am doing the doc updates.

- versionchanged for pbc -> wrap everywhere - fixed format of older versionchanged markup - added missing docs for moment_of_inertia() and replaced kwargs with explicit keyword arguments - removed superfluous kwargs from shape_parameter()

orbeckst · 2022-03-05T16:18:53Z

I hope the tests pass now. I am approving now because I might not be able to get to it later during the day and in principle at least everything is there. Another pair of 👀 would be good before merging.

richardjgowers

Lgtm!

richardjgowers commented Jul 19, 2020

View reviewed changes

jbarnoud reviewed Jul 19, 2020

View reviewed changes

orbeckst requested changes Jul 29, 2020

View reviewed changes

orbeckst self-assigned this Jul 29, 2020

IAlibay mentioned this pull request Jan 24, 2021

Release 1.0.1 #2768

Closed

IAlibay added this to the 2.0 milestone Apr 6, 2021

IAlibay requested a review from mnmelo April 26, 2021 19:59

orbeckst requested changes Apr 28, 2021

View reviewed changes

orbeckst modified the milestones: 2.0, 2.1.0 Aug 17, 2021

richardjgowers and others added 3 commits February 10, 2022 18:03

renamed 'pbc' kwarg to 'wrap' for Group methods

94590a6

issue #1760

added CHANGELOG and deprecation warning for pbc kwarg

cbc5980

Fix some changes missed during the rebase

e9b5185

Rename a missed variable and remove calls that use both wrap and unwrap. This commit does not pass the tests because some of them rely on both wrap and unwrap in the same call. This will be fixed in a subsequent commit.

jbarnoud force-pushed the pbc_to_wrap branch from 4bf06ef to e9b5185 Compare February 15, 2022 12:22

github-actions bot added the Component-Core label Feb 15, 2022

Fix a broken test

722b475

topologyobjects.Bond.length still has a pdb argument. This commit reflects this.

jbarnoud added 3 commits March 3, 2022 10:34

Fix tests that used both wrap and unwrap

e8d992a

Update deprecation notice about pbc->wrap

fa1b344

Fix PEP8

1bbd202

Ignoed expected lint warnings

b1eb7d0

Testing deprecated arguments lead to false positives in pylint. This commit ignores the specific warning on the problematic lines.

jbarnoud mentioned this pull request Mar 4, 2022

Release 2.1.0 #3446

Closed

5 tasks

orbeckst requested changes Mar 4, 2022

View reviewed changes

package/MDAnalysis/core/topologyattrs.py Show resolved Hide resolved

package/MDAnalysis/core/topologyattrs.py Show resolved Hide resolved

orbeckst added 2 commits March 4, 2022 18:09

updated docs

52b5539

- versionchanged for pbc -> wrap everywhere - fixed format of older versionchanged markup - added missing docs for moment_of_inertia() and replaced kwargs with explicit keyword arguments - removed superfluous kwargs from shape_parameter()

added docs for tensor moment of inertia

9082d4e

orbeckst force-pushed the pbc_to_wrap branch from 3de5373 to 9082d4e Compare March 5, 2022 01:51

orbeckst approved these changes Mar 5, 2022

View reviewed changes

orbeckst force-pushed the pbc_to_wrap branch 2 times, most recently from d76d3b7 to 75420b6 Compare March 5, 2022 16:24

fixed moment of inertia doc string

bf4bfd7

orbeckst force-pushed the pbc_to_wrap branch from 75420b6 to bf4bfd7 Compare March 5, 2022 16:25

richardjgowers commented Mar 5, 2022

View reviewed changes

jbarnoud merged commit 8835ab7 into develop Mar 6, 2022

jbarnoud deleted the pbc_to_wrap branch March 6, 2022 14:31

fiona-naughton added the defect label Sep 25, 2023

Conversation

richardjgowers commented Jul 19, 2020 • edited by jbarnoud Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Checklist

Uh oh!

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

Uh oh!

Comment last updated at 2022-03-05 16:25:15 UTC

Uh oh!

richardjgowers Jul 19, 2020

Choose a reason for hiding this comment

Uh oh!

richardjgowers commented Jul 19, 2020

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

jbarnoud commented Jul 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jbarnoud Jul 19, 2020

Choose a reason for hiding this comment

Uh oh!

orbeckst commented Jul 21, 2020

Uh oh!

richardjgowers commented Jul 21, 2020

Uh oh!

orbeckst commented Jul 21, 2020

Uh oh!

orbeckst 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!

IAlibay commented Apr 6, 2021

Uh oh!

orbeckst commented Apr 6, 2021

Uh oh!

orbeckst commented Apr 27, 2021

Uh oh!

IAlibay commented Apr 27, 2021

Uh oh!

orbeckst left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

orbeckst commented May 11, 2021

Uh oh!

IAlibay commented May 11, 2021

Uh oh!

IAlibay commented Aug 17, 2021

Uh oh!

orbeckst commented Aug 17, 2021

Uh oh!

orbeckst commented Oct 28, 2021

Uh oh!

jbarnoud commented Feb 4, 2022

Uh oh!

jbarnoud commented Feb 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

richardjgowers commented Feb 16, 2022

Uh oh!

jbarnoud commented Feb 16, 2022

Uh oh!

jbarnoud commented Mar 3, 2022

Uh oh!

orbeckst 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!

richardjgowers commented Jul 19, 2020 •

edited by jbarnoud

Loading

pep8speaks commented Jul 19, 2020 •

edited

Loading

codecov bot commented Jul 19, 2020 •

edited

Loading

jbarnoud commented Jul 19, 2020 •

edited

Loading

orbeckst left a comment •

edited

Loading

jbarnoud commented Feb 15, 2022 •

edited

Loading

orbeckst left a comment •

edited

Loading