Skip to content

Support version 0.4 of SMIRNOFF Electrostatics section #1277

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 18 commits into from
May 6, 2022
Merged

Support version 0.4 of SMIRNOFF Electrostatics section #1277

merged 18 commits into from
May 6, 2022

Conversation

@mattwthompson
Copy link
Member

@mattwthompson mattwthompson commented Apr 20, 2022 •
edited
Loading

Resolves #1084 in part

Fixes #1288

  • Blocked by OFF-EP 0005b standards#34
  • Add up-converters
  • Test up-converters
  • Handle switch_width in electrostatics handler Do not handle, but explicitly do not handle
  • Band-aid old code paths?
  • Update Interchange - I'd like to do this after this PR to avoid too many feature branches being split off
  • Tag issue being addressed
  • Add tests
  • Update docstrings/documentation, if applicable - I did a scan for .method and similar patterns and didn't find anything obviously outdated.
  • Lint codebase
  • Update changelog

@codecov
Copy link

codecov bot commented Apr 20, 2022 •
edited
Loading

Codecov Report

Merging #1277 (1d7e1d7) into topology-biopolymer-refactor (8128d0b) will decrease coverage by 2.54%.
The diff coverage is 96.05%.

@mattwthompson mattwthompson marked this pull request as ready for review April 27, 2022 13:20
@mattwthompson mattwthompson mentioned this pull request Apr 27, 2022
Closed
@mattwthompson mattwthompson requested a review from j-wags April 27, 2022 13:54
@mattwthompson mattwthompson mentioned this pull request Apr 27, 2022
Merged
j-wags
j-wags requested changes May 5, 2022
View reviewed changes
Copy link
Member

@j-wags j-wags left a 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.

j-wags
j-wags approved these changes May 5, 2022
View reviewed changes
Copy link
Member

@j-wags j-wags left a 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.

@mattwthompson mattwthompson merged commit 0107544 into topology-biopolymer-refactor May 6, 2022
@mattwthompson mattwthompson deleted the prep-off-ep-0005b branch May 6, 2022 02:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@j-wags j-wags j-wags approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@mattwthompson @j-wags