Skip to content

Add elements attribute to TPR parser#2858

Merged
lilyminium merged 8 commits intodevelopfrom
tpr_elements
Jul 21, 2020
Merged

Add elements attribute to TPR parser#2858
lilyminium merged 8 commits intodevelopfrom
tpr_elements

Conversation

@jbarnoud
Copy link
Contributor

@jbarnoud jbarnoud commented Jul 19, 2020

Elements are read from the TPR. TPR files contain the atomic number for
each atoms, when an atom does not match an element (some virtual sites,
coarse-grained particles), the atomic number is set to -1.

The TPR parser adds the 'elements' attribute only if at least 1 atom has
its atomic number corresponding to an element. In that case, atoms that
do not match an element have their element set to an empty string.

See #2553
Closes #2628

PR Checklist

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

Elements are read from the TPR. TPR files contain the atomic number for
each atoms, when an atom does not match an element (some virtual sites,
coarse-grained particles), the atomic number is set to -1.

The TPR parser adds the 'elements' attribute only if at least 1 atom has
its atomic number correspomding to an element. In that case, atoms that
do not match an element have theire element set to an empty string.

See #2553
Closes #2628
@pep8speaks
Copy link

pep8speaks commented Jul 19, 2020

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

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2020-07-20 10:22:39 UTC

Copy link
Member

@IAlibay IAlibay left a comment

Choose a reason for hiding this comment

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

Could a test also be added that checks that the right elements have been assigned?

@codecov
Copy link

codecov bot commented Jul 19, 2020

Codecov Report

Merging #2858 into develop will increase coverage by 0.06%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #2858      +/-   ##
===========================================
+ Coverage    92.70%   92.77%   +0.06%     
===========================================
  Files          185      185              
  Lines        24154    24676     +522     
  Branches      3128     3133       +5     
===========================================
+ Hits         22391    22892     +501     
- Misses        1717     1738      +21     
  Partials        46       46              
Impacted Files Coverage Δ
package/MDAnalysis/topology/TPRParser.py 95.16% <ø> (ø)
package/MDAnalysis/topology/tpr/obj.py 96.96% <100.00%> (+0.24%) ⬆️
package/MDAnalysis/topology/tpr/utils.py 96.96% <100.00%> (+0.02%) ⬆️
package/MDAnalysis/analysis/rms.py 95.06% <0.00%> (-0.34%) ⬇️
coordinates/base.py 94.82% <0.00%> (+0.28%) ⬆️
auxiliary/base.py 91.31% <0.00%> (+0.56%) ⬆️
package/MDAnalysis/lib/util.py 90.02% <0.00%> (+1.31%) ⬆️
util.py 90.11% <0.00%> (+2.05%) ⬆️

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 c7a6b62...376fa2c. Read the comment docs.

@jbarnoud
Copy link
Contributor Author

Could a test also be added that checks that the right elements have been assigned?

I was afraid somebody would ask... Done now.

Copy link
Member

@IAlibay IAlibay left a comment

Choose a reason for hiding this comment

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

I was afraid somebody would ask... Done now.

Y'all taught us too well :)

As far as I understand the TPR parser, lgtm!

* Added Hydrogen Bond Lifetime via existing autocorrelation features (PR #2791)
* Added Hydrogen Bond Lifetime keyword "between" (PR #2791)
* Dead code removed from the TPR parser and increased test coverage (PR #2840)
* TPR parser exposes the elements topology attribute (PR #2858)
Copy link
Member

Choose a reason for hiding this comment

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

Issue number could be useful here too.

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.

LGTM, but I think there's a typo in the property docstring :-)

* Atoms: number, name, type, resname, resid, segid, mass, charge, element
[residue, segment, radius, bfactor, resnum, moltype]
* Bonds
* Angels
Copy link
Member

Choose a reason for hiding this comment

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

😇

"""
The symbol of the atom element.

Returns
Copy link
Member

Choose a reason for hiding this comment

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

It's odd to think of a property "returning" something, but on the other hand very few people will see this

jbarnoud and others added 4 commits July 20, 2020 09:14
@jbarnoud
Copy link
Contributor Author

@lilyminium @IAlibay Thank you for the review. I think I addressed all your comments.

@orbeckst
Copy link
Member

@IAlibay @lilyminium thank you for reviewing. I just put you down as the official assignees so one of you has the honor of merging.

@lilyminium lilyminium merged commit 2550d06 into develop Jul 21, 2020
@lilyminium lilyminium deleted the tpr_elements branch July 21, 2020 12:02
PicoCentauri pushed a commit to PicoCentauri/mdanalysis that referenced this pull request Mar 30, 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.

6 participants