-
Notifications
You must be signed in to change notification settings - Fork 95
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
Support version 0.4 of SMIRNOFF Electrostatics section #1277
Support version 0.4 of SMIRNOFF Electrostatics section #1277
Conversation
Codecov Report
|
a681229
to
0ea1d7a
Compare
…into prep-off-ep-0005b
…into prep-off-ep-0005b
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.
Ok, I hammered on this a bit and it looks like everything works well. The only thing this PR still needs to add the solvent_dielectric
ParameterAttribute required in the 0.4 Electrostatics spec. Per the spec, it should default to "none", and it's fine if we have the setter just raise a NotImplementedError
if someone tries to set it to anything else.
Also (per inline comment) I don't think we should even try to support unexpected 0.3 methods
- It would be fine if anything fishy just raises an error.
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.
Nice - Thanks @mattwthompson! Feel free to merge once tests pass.
Resolves #1084 in part
Fixes #1288
Blocked byOFF-EP 0005b standards#34HandleDo not handle, but explicitly do not handleswitch_width
in electrostatics handlerUpdate Interchange- I'd like to do this after this PR to avoid too many feature branches being split off.method
and similar patterns and didn't find anything obviously outdated.