Skip to content
New issue

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

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

Already on GitHub? # to your account

Read SMIRFF-enclosed FFs #371

Merged
merged 9 commits into from
Jul 2, 2019
Merged

Read SMIRFF-enclosed FFs #371

merged 9 commits into from
Jul 2, 2019

Conversation

j-wags
Copy link
Member

@j-wags j-wags commented Jun 28, 2019

Summary

In testing the ability to read all files from a release-candidate smirnoff99Frosst package, I found that ones with an enclosing XML SMIRFF tag (instead of SMIRNOFF) were unable to be read. This PR fixes that.

@j-wags j-wags changed the title Read smirff Read SMIRFF-enclosed FFs Jun 29, 2019
@j-wags j-wags requested a review from andrrizzi June 29, 2019 00:37
@codecov-io
Copy link

codecov-io commented Jun 29, 2019

Codecov Report

Merging #371 into master will increase coverage by 0.04%.
The diff coverage is 90.9%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #371      +/-   ##
==========================================
+ Coverage   76.93%   76.97%   +0.04%     
==========================================
  Files          19       19              
  Lines        5414     5416       +2     
==========================================
+ Hits         4165     4169       +4     
+ Misses       1249     1247       -2
Impacted Files Coverage Δ
...enforcefield/typing/engines/smirnoff/forcefield.py 75.36% <90.9%> (+0.14%) ⬆️
openforcefield/utils/utils.py 84.66% <0%> (+0.66%) ⬆️

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 89b1f1a...f43843e. Read the comment docs.

Copy link
Collaborator

@andrrizzi andrrizzi left a comment

Choose a reason for hiding this comment

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

Looks good, thank you! I've noticed a possible bug, but I might be missing something because your new test is passing. Feel free to merge when ready.

The only main suggestion I have is to update data/test_forcefields/README.md to briefly describe the two new force field files. I think it'll help to keep track of the differences between the different files, which will probably become difficult to do without some notes in there.

@davidlmobley
Copy link
Contributor

@j-wags Are you sure we don't want to just get those tags converted? Long long ago we called them "SMIRFF" forcefields rather than "SMIRNOFF", but finally decided to change the name. However, the intention wasn't to maintain backwards compatibility, just to switch.

@j-wags
Copy link
Member Author

j-wags commented Jul 1, 2019

@davidlmobley

My goal here is to have every FF from the smirnoff99Frosst repo loadable, and all of them up to the 1.0.4(?) release are enclosed with a SMIRFF tag. Do you foresee any danger in loading those? I'm under the impression that the format in those files is identical to the "official" 0.1 SMIRNOFF spec.

@j-wags j-wags mentioned this pull request Jul 1, 2019
2 tasks
@j-wags
Copy link
Member Author

j-wags commented Jul 1, 2019

@davidlmobley To clarify -- I want the toolkit to read SMIRFF-enclosed files because the alternatives are:

  1. modifying the released smirnoff99Frosst files, which we probably agree is bad, or
  2. not being able to read all released smirnoff99Frosst files using the current toolkit. I wasn't under the impression that there was much risk in auto-upgrading the spec of these old files, but if you suspect that there is, we could remove (or do a deep dive into) this functionality.

@j-wags j-wags merged commit 0aa73d6 into master Jul 2, 2019
@j-wags j-wags deleted the read_smirff branch July 2, 2019 15:34
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants