Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Parsing negative values in ATOMIC_NUMBER records of AMBER topologies #2307

Merged
merged 4 commits into from Aug 2, 2019

Conversation

IAlibay
Copy link
Member

@IAlibay IAlibay commented Aug 1, 2019

Fixes #2306

Changes made in this Pull Request:

Questions:

  • Do we want to be warning the users in TOPParser or within the guessers?

PR Checklist

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

Copy link
Member

@richardjgowers richardjgowers left a comment

Choose a reason for hiding this comment

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

Thanks @IAlibay , looks good, is it easy to add a quick test checking that a warning happens? If it's tricky I'll just merge as is

@@ -348,6 +349,40 @@ class TestPRMNCRST(TOPBase):
atom_i_improper_values = ()


class TestPRMNCRST_negative(TOPBase):
Copy link
Member

Choose a reason for hiding this comment

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

you should be able to write some sort of test that uses assert_warns (or is it with pytest.warns:?)

@codecov
Copy link

codecov bot commented Aug 1, 2019

Codecov Report

Merging #2307 into develop will decrease coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #2307      +/-   ##
===========================================
- Coverage    89.63%   89.61%   -0.02%     
===========================================
  Files          173      157      -16     
  Lines        21498    19525    -1973     
  Branches      2801     2803       +2     
===========================================
- Hits         19269    17498    -1771     
+ Misses        1628     1431     -197     
+ Partials       601      596       -5
Impacted Files Coverage Δ
package/MDAnalysis/topology/TOPParser.py 100% <100%> (ø) ⬆️
...age/MDAnalysis/analysis/hbonds/hbond_autocorrel.py 87.8% <0%> (-0.54%) ⬇️
core/util.py
transformations/__init__.py
auxiliary/base.py
util.py
topology/base.py
__init__.py
topology/__init__.py
visualization/__init__.py
... and 16 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 89a9514...8a1295e. Read the comment docs.

@IAlibay
Copy link
Member Author

IAlibay commented Aug 1, 2019

@richardjgowers Thanks for the review, hopefully 8a1295e should cover the warnings 😄

Unfortunately I couldn't find a quick way to not have to duplicate some of the functions.

Copy link
Member

@richardjgowers richardjgowers left a comment

Choose a reason for hiding this comment

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

@IAlibay yep thanks!

@orbeckst orbeckst merged commit 7449a38 into MDAnalysis:develop Aug 2, 2019
@orbeckst
Copy link
Member

orbeckst commented Aug 2, 2019

I ignored AppVeyor and merged. Thanks @IAlibay !

@IAlibay IAlibay deleted the NegativeTop branch August 5, 2019 09:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AMBER topology files with negative ATOMIC_NUMBER fail to be parsed
3 participants