-
Notifications
You must be signed in to change notification settings - Fork 81
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
Allow parsing of offxml strings #288
Conversation
Confirmed this works when running the solvation leg of a benchmark free energy calculation via openfe. |
@jthorton I did a merge commit to bring in a fix that was needed, let me know if it looks okay! |
Thanks @mikemhenry looks like it changed the exception message slightly which is fine for me. Just want to check if my use of the walrus |
I am happy with using python 3.8+ language features. We can even use python 3.9+ language features since we follow NEP29 (or try to) |
OpenFF is pretty good about using it, so congrats - you've inherited it! |
Ah classic, can't run this from a fork. @jthorton I will push this to a branch in the repo so we can make sure it passes in CI |
@jthorton I don't know if this trick will keep working if you make a commit, so ping me if you do and CI stops running |
Looks like it passes 🚀 @ijpulidos would you mind reviewing this one? Should we wait for #293 first? |
I don't think we need to wait on any other PRs, just need another review from @ijpulidos |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking great! I just added some comment about the error message when you get a poorly formatted string.
self._smirnoff_forcefield = openff.toolkit.typing.engines.smirnoff.ForceField(forcefield) | ||
except Exception as e: | ||
_logger.error(e) | ||
raise ValueError(f"Can't find specified SMIRNOFF force field ({forcefield}) in install paths") from e |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we want to improve the error message here, since as far as I can see it's not now that we don't find the ff on the install paths but it's rather that the provided string is not correctly formatted as a ffxml string.
Is it too crazy to add some more logic here such that we can have specific error messages as to what is causing this? It doesn't seem too hard to accomplish, as far as I can tell.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have updated the error to reflect that a missing file or poorly formatted string could cause this error. As for specific messages on the formatting of the forcefield I would prefer to leave this to the openff.toolkit.ForceField
and just raise the error from there as it's quite descriptive itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
This PR allows an offxml string to be used as a small molecule force field for the
SystemGenerator
andSMIRNOFFTemplateGenerator
see the example which now works with this PR.