Add kwarg to index TPRParser from 0 or 1 by lilyminium · Pull Request #3152 · MDAnalysis/mdanalysis

lilyminium · 2021-03-13T04:25:15Z

Changes made in this Pull Request:

Added keyword argument to index TPRParser from 0 or 1
Set the default to index from 1, changing the default behaviour

This is easily undoable. I read the discussion in #2364 as supporting both including a kwarg and a warning. If that is not consensus, please let me know. I'll update CHANGELOG once we have this nailed down :-)

PR Checklist

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

pep8speaks · 2021-03-13T04:25:20Z

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

In the file testsuite/MDAnalysisTests/analysis/test_density.py:

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

Comment last updated at 2021-03-14 17:05:53 UTC

codecov · 2021-03-13T04:43:35Z

Codecov Report

Merging #3152 (76f6cdf) into develop (cadf5b9) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff            @@
##           develop    #3152   +/-   ##
========================================
  Coverage    92.68%   92.68%           
========================================
  Files          168      168           
  Lines        22677    22679    +2     
  Branches      3212     3213    +1     
========================================
+ Hits         21018    21020    +2     
  Misses        1611     1611           
  Partials        48       48

Impacted Files	Coverage Δ
package/MDAnalysis/topology/TPRParser.py	`95.16% <100.00%> (ø)`
package/MDAnalysis/topology/tpr/utils.py	`96.98% <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 cadf5b9...76f6cdf. Read the comment docs.

jbarnoud

Looks good to me. I am not sure the deprecation message is very clear, though. I would phrase the message to say that from version 2.0, resid will start at 1 rather than 0.

jbarnoud

I just realized it is missing an entry in the changelog and a "versionchanged".

lilyminium · 2021-03-13T18:32:48Z

Thanks @jbarnoud, I've updated CHANGELOG now. I also realised this PR is to develop, so I removed the deprecation warning and changed the default. #3153 is the "first" addition of this to master with the deprecation warning and the status quo default of tpr_resid_from_one=False.

orbeckst

Thank you! I was first confused by the PR description but then saw that this is one against develop.

I have minor improvements and one bigger discussion point regarding the default value inside the low-level TPR reader code.

package/MDAnalysis/topology/tpr/utils.py

package/CHANGELOG

orbeckst · 2021-03-14T00:46:49Z

package/MDAnalysis/topology/tpr/utils.py



-def do_mtop(data, fver):
+def do_mtop(data, fver, tpr_resid_from_one=True):


I feel the default should remain False because that is what the TPR actually contains. IMHO the TPR functions themselves could/should stay as faithful to the original format as possible. Assume someone wrote their own TPR parser with our functions. With the new default you break their code needlessly, with the old defaults nothing changes and our own parser just makes the well-documented choice to index from 1.

I can be convinced otherwise (ie to let default True stand) but wanted to discuss this point first before approving.

Others may want to chime in (maybe the most knowledgeable, @jbarnoud) but I agree with your argument.

I am going to approve because it looks sensible from my point but let @jbarnoud also say something before merging.

TPR indeed index from 0, but it is a bit moot because you do not read a TPR directly. Instead, you end up reading your gro and ITP files to make sense of your TPR so consistency there seems the right thing to do.

My previous objection was that it would break the existing code, but the deprecation in 1.1.0 should be enough.

orbeckst · 2021-03-14T03:10:29Z

@lilyminium the issue description is not correct, is it?

Set the default to index from 0, i.e. status quo

This PR (against develop) does change the default indexing to 1-based.

orbeckst · 2021-03-14T06:04:15Z

Note that anything referring to 1.0.2 should become 1.1.0 (see comment on #3153 ).

Co-authored-by: Oliver Beckstein <orbeckst@gmail.com>

orbeckst · 2021-03-14T22:16:57Z

@jbarnoud any last comments? Otherwise I'll merge tomorrow.

- Fixes #2364 - Master version of #3152. - Changes made in this Pull Request: - Add `tpr_resid_from_one` keyword to TPRParser - Set default to `False`, keeping current status quo - Emits a warning for `tpr_resid_from_one=False` indicating that default behaviour will change in 2.0 - add tests - update CHANGELOG

* add kwarg * change default to True for 2.0 * fix tests to index resids from 0 again * Update package/CHANGELOG Co-authored-by: Oliver Beckstein <orbeckst@gmail.com> * change default arg for TPR utilities and remove warnings * updated to 1.1.0 Co-authored-by: Oliver Beckstein <orbeckst@gmail.com>

lilyminium requested review from IAlibay, jbarnoud and orbeckst March 13, 2021 04:25

jbarnoud approved these changes Mar 13, 2021

View reviewed changes

jbarnoud requested changes Mar 13, 2021

View reviewed changes

lilyminium force-pushed the fix-2364 branch from f01fa49 to 5c6886c Compare March 13, 2021 18:19

lilyminium mentioned this pull request Mar 13, 2021

Add tpr_resid_from_one keyword to TPRParser #3153

Merged

4 tasks

lilyminium requested a review from jbarnoud March 13, 2021 18:31

lilyminium force-pushed the fix-2364 branch from 3711d0b to 856f035 Compare March 14, 2021 00:18

orbeckst requested changes Mar 14, 2021

View reviewed changes

orbeckst approved these changes Mar 14, 2021

View reviewed changes

lilyminium force-pushed the fix-2364 branch from c0137dd to cfc49d3 Compare March 14, 2021 17:01

lilyminium and others added 6 commits March 14, 2021 10:04

add kwarg

4fe5f20

change default to True for 2.0

3b92869

fix tests to index resids from 0 again

a38a651

Update package/CHANGELOG

b1844eb

Co-authored-by: Oliver Beckstein <orbeckst@gmail.com>

change default arg for TPR utilities and remove warnings

a77e445

updated to 1.1.0

76f6cdf

lilyminium force-pushed the fix-2364 branch from cfc49d3 to 76f6cdf Compare March 14, 2021 17:05

orbeckst self-assigned this Mar 14, 2021

orbeckst mentioned this pull request Mar 14, 2021

TPRParser should not index from 0 #2364

Closed

IAlibay added this to the 2.0 milestone Mar 14, 2021

jbarnoud approved these changes Mar 15, 2021

View reviewed changes

jbarnoud merged commit 6282385 into MDAnalysis:develop Mar 15, 2021

IAlibay added the defect label Sep 25, 2023



		def do_mtop(data, fver):
		def do_mtop(data, fver, tpr_resid_from_one=True):

Conversation

lilyminium commented Mar 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Checklist

Uh oh!

pep8speaks commented Mar 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Comment last updated at 2021-03-14 17:05:53 UTC

Uh oh!

codecov bot commented Mar 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

jbarnoud left a comment

Choose a reason for hiding this comment

Uh oh!

jbarnoud left a comment

Choose a reason for hiding this comment

Uh oh!

lilyminium commented Mar 13, 2021

Uh oh!

orbeckst left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

orbeckst Mar 14, 2021

Choose a reason for hiding this comment

Uh oh!

orbeckst Mar 14, 2021

Choose a reason for hiding this comment

Uh oh!

lilyminium Mar 14, 2021

Choose a reason for hiding this comment

Uh oh!

orbeckst Mar 14, 2021

Choose a reason for hiding this comment

Uh oh!

jbarnoud Mar 15, 2021

Choose a reason for hiding this comment

Uh oh!

jbarnoud Mar 15, 2021

Choose a reason for hiding this comment

Uh oh!

orbeckst commented Mar 14, 2021

Uh oh!

orbeckst commented Mar 14, 2021

Uh oh!

orbeckst commented Mar 14, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

lilyminium commented Mar 13, 2021 •

edited

Loading

pep8speaks commented Mar 13, 2021 •

edited

Loading

codecov bot commented Mar 13, 2021 •

edited

Loading