Skip to content

Add tpr_resid_from_one keyword to TPRParser#3153

Merged
orbeckst merged 4 commits intoMDAnalysis:masterfrom
lilyminium:fix-2364-master
Mar 14, 2021
Merged

Add tpr_resid_from_one keyword to TPRParser#3153
orbeckst merged 4 commits intoMDAnalysis:masterfrom
lilyminium:fix-2364-master

Conversation

@lilyminium
Copy link
Member

@lilyminium lilyminium commented Mar 13, 2021

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

PR Checklist

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

@codecov
Copy link

codecov bot commented Mar 13, 2021

Codecov Report

Merging #3153 (ee03c5b) into master (7ed893d) will increase coverage by 0.84%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3153      +/-   ##
==========================================
+ Coverage   90.97%   91.81%   +0.84%     
==========================================
  Files         162      167       +5     
  Lines       22194    22703     +509     
  Branches     3198     3199       +1     
==========================================
+ Hits        20190    20845     +655     
- Misses       1381     1774     +393     
+ Partials      623       84     -539     
Impacted Files Coverage Δ
package/MDAnalysis/topology/TPRParser.py 93.93% <100.00%> (+6.06%) ⬆️
package/MDAnalysis/topology/tpr/utils.py 80.97% <100.00%> (+6.61%) ⬆️
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/analysis/legacy/__init__.py 0.00% <0.00%> (ø)
package/MDAnalysis/tests/datafiles.py 31.25% <0.00%> (ø)
package/MDAnalysis/tests/__init__.py 100.00% <0.00%> (ø)
package/MDAnalysis/core/selection.py 99.52% <0.00%> (+<0.01%) ⬆️
... 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 7ed893d...ee03c5b. Read the comment docs.

Copy link
Member

@orbeckst orbeckst left a comment

Choose a reason for hiding this comment

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

Thank you.

The CHANGELOG entries are in the wrong spot (1.0.0?).

The introduction of the tpr_resid_from_one keyword forces us to make this 1.1.0 according to https://semver.org/ (ping @IAlibay ).

Comment on lines +287 to +288
* TPRParser indexing resids from 0 by default is deprecated in 1.0.
From 2.0 TPRParser will index resids from 1 by default.
Copy link
Member

Choose a reason for hiding this comment

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

In the wrong spot.

Comment on lines +189 to +191
.. versionchanged:: 1.0.2
Added the ``tpr_resid_from_one`` keyword to control if
resids are indexed from 0 or 1. Default ``False``.
Copy link
Member

Choose a reason for hiding this comment

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

Becomes 1.1.0 ...

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought we were going to leave 1.x at the 1.0.x states? It's becoming increasingly difficult to deal with Python 2.7, as evidenced by the recent issues with CI. What about putting the deprecation in 2.0 and then making the final change in a later release?

Tagging any interested @MDAnalysis/coredevs.

@IAlibay IAlibay mentioned this pull request Mar 14, 2021
9 tasks
@orbeckst orbeckst merged commit 03c6317 into MDAnalysis:master Mar 14, 2021
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.

3 participants