Skip to content

Removes element guessing from ParmedParser (Issue #2933)#2959

Merged
lilyminium merged 4 commits intoMDAnalysis:developfrom
IAlibay:issue-2933
Sep 30, 2020
Merged

Removes element guessing from ParmedParser (Issue #2933)#2959
lilyminium merged 4 commits intoMDAnalysis:developfrom
IAlibay:issue-2933

Conversation

@IAlibay
Copy link
Copy Markdown
Member

@IAlibay IAlibay commented Sep 29, 2020

Fixes #2933

Changes made in this Pull Request:

  • If elements are not present in the parmed structure, the element is assigned to empty string ''.
  • Tests for ParmedParser element reading.
  • Some PEP8/Flake8 changes.

PR Checklist

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

@IAlibay IAlibay requested a review from lilyminium September 29, 2020 19:19

expected_n_atoms = 252
expected_n_residues = 14
elems_ranges = ((0, 8), (30, 38))
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

So strangely enough Parmed here actually guesses elements from masses (hence why PRM directly loaded in MDA has no elements, but this yields an element list). But since it's outside our hands... I guess it's fine?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I agree, IMO the goal should be to faithfully represent the ParmEd object, however it chooses to interpret the original file.

@codecov
Copy link
Copy Markdown

codecov bot commented Sep 29, 2020

Codecov Report

Merging #2959 into develop will decrease coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #2959      +/-   ##
===========================================
- Coverage    93.05%   93.05%   -0.01%     
===========================================
  Files          186      186              
  Lines        24613    24609       -4     
  Branches      3187     3187              
===========================================
- Hits         22904    22900       -4     
  Misses        1661     1661              
  Partials        48       48              
Impacted Files Coverage Δ
package/MDAnalysis/topology/ParmEdParser.py 98.44% <100.00%> (-0.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 2687c40...eb247c1. Read the comment docs.

Copy link
Copy Markdown
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! Thanks for doing this and cleaning up the code 😅


expected_n_atoms = 252
expected_n_residues = 14
elems_ranges = ((0, 8), (30, 38))
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I agree, IMO the goal should be to faithfully represent the ParmEd object, however it chooses to interpret the original file.

@lilyminium lilyminium merged commit 9b88b83 into MDAnalysis:develop Sep 30, 2020
PicoCentauri pushed a commit to PicoCentauri/mdanalysis that referenced this pull request Mar 30, 2021
…DAnalysis#2959)

* assigns empty string for missing elements
* PEP8 fixes
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.

Remove element guessing by default in ParmedParser

3 participants