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

Collapsing combining of Forcefield when loading from multiple files #442

Merged
merged 12 commits into from
Aug 7, 2021

Conversation

daico007
Copy link
Member

@daico007 daico007 commented Jul 7, 2021

PR Summary:

Relate to #440 and #441. Enforcing that combined forcefield (loaded from multiple files) must have the same combining rule. Then collapse the combining rule to a single string (fix the bug found in #440). Also, remove the combining_rule option in the apply step.

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 Jul 7, 2021

Codecov Report

Merging #442 (165b6c4) into master (af0cb08) will increase coverage by 0.04%.
The diff coverage is 87.50%.

❗ Current head 165b6c4 differs from pull request most recent head 0a18fb5. Consider uploading reports for the commit 0a18fb5 to get more accurate results

@@            Coverage Diff             @@
##           master     #442      +/-   ##
==========================================
+ Coverage   73.84%   73.88%   +0.04%     
==========================================
  Files          17       17              
  Lines        1839     1842       +3     
==========================================
+ Hits         1358     1361       +3     
  Misses        481      481              

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, i think this will lead to foyer 0.10.0 in my opinion. Since we are making a pretty big change to the foyer apply argument, this will be a breaking change.

@daico007 daico007 merged commit 78dbdaf into mosdef-hub:master Aug 7, 2021
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.

2 participants