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

fixed error in verbose mode for missing dihedrals #335

Merged
merged 3 commits into from
Jun 12, 2020
Merged

fixed error in verbose mode for missing dihedrals #335

merged 3 commits into from
Jun 12, 2020

Conversation

chrisiacovella
Copy link
Contributor

PR Summary:

This is relate to a few recent issues posted.

#326
#323

  1. There was a coding issue in the part of the code that would write out a missing RB dihedral when in verbose mode (undefined parameters referenced). Also, only ids were being output, not types (inconsistent with, e.g., how angles were outputted). The coding and formatting issues have been addressed.

  2. Even when fixing that coding/formatting issues, the code only checked the RB torsions. So, when I was testing periodic torsions, it alerted that all dihedrals were missing. This bit now has two loops, first looping over proper_dihedrals and then rb_torsions in parmed structure.

PR Checklist


  • Includes appropriate unit test(s)
  • Appropriate docstring(s) are added/updated
  • Code is (approximately) PEP8 compliant
  • Issue(s) raised/addressed?

@codecov
Copy link

codecov bot commented Jun 4, 2020

Codecov Report

Merging #335 into master will decrease coverage by 0.22%.
The diff coverage is 50.00%.

@@            Coverage Diff             @@
##           master     #335      +/-   ##
==========================================
- Coverage   84.78%   84.55%   -0.23%     
==========================================
  Files          14       14              
  Lines        1275     1282       +7     
==========================================
+ Hits         1081     1084       +3     
- Misses        194      198       +4     

@rsdefever
Copy link
Member

Hey @umesh-timalsina @justinGilmer looks like we are having some issues with the tests not running here as well.

@umesh-timalsina
Copy link
Member

@rsdefever This branch needs a merge with the latest commit added by #334. The required checks have been modified.

@rsdefever
Copy link
Member

@rsdefever This branch needs a merge with the latest commit added by #334. The required checks have been modified.

@chrisiacovella FYI

Copy link
Contributor

@justinGilmer justinGilmer left a comment

Choose a reason for hiding this comment

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

Looking great, thanks @chrisiacovella ! A few questions/statements.

Can you review Ray’s PR #327 and see if there are other features included there that might be useful in verbose mode?

Also, can you merge the latest upstream master changes into this branch so we can check the tests? Some will fail due to #337 not being merged, but that is ok.

Assuming you have the mosdef-hub/foyer remote named origin

git fetch upstream
git merge upstream master
# assuming your remote is named origin
git push origin 

@chrisiacovella
Copy link
Contributor Author

Totally didn't see Ray's PR when I put this together. I merged in his additions to the error messages. I think we are good to go whenever these checks complete. I'll merge it at that point.

Copy link
Contributor

@justinGilmer justinGilmer left a comment

Choose a reason for hiding this comment

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

lgtm! @rmatsum836 or @daico007 can one of yall give it a once over? After that, its ready to merge.

Copy link
Member

@daico007 daico007 left a comment

Choose a reason for hiding this comment

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

LGTM

@daico007 daico007 merged commit ff6b66d into mosdef-hub:master Jun 12, 2020
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.

5 participants