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

OFF-EP 0005 #30

Closed
wants to merge 10 commits into from
Closed

OFF-EP 0005 #30

wants to merge 10 commits into from

Conversation

mattwthompson
Copy link
Member

Resolves #29

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.

I approve this OFF EP as written. Thanks for your careful work proposing and drafting this change, @mattwthompson!

docs/enhancement-proposals/off-ep-0005.md Outdated Show resolved Hide resolved
Co-authored-by: Jeff Wagner <jwagnerjpl@gmail.com>
Copy link

@davidlmobley davidlmobley left a comment

Choose a reason for hiding this comment

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

Approved! However, please make minor grammatical fixes/clarifications as noted.

docs/enhancement-proposals/off-ep-0005.md Outdated Show resolved Hide resolved
docs/enhancement-proposals/off-ep-0005.md Outdated Show resolved Hide resolved
<Electrostatics version="0.3" method="PME" scale12="0.0" scale13="0.0" scale14="0.833333" scale15="1.0"/>
```

to the equivalent of reading

Choose a reason for hiding this comment

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

Not sure this is grammatically correct; should it be "to the less ambiguous new version, which would read" or something? It depends on what you mean here and i find it at least confusing and possibly not grammatically correct.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm more or less trying to say how the toolkit will/should handle up-conversions of this snippet, which I'm pretty sure is the only one that's out in the wild. I can see I was being a little ambiguous in thinking about the in-memory representation where simply saying what is should look like post up-conversion would be simpler.

@jchodera
Copy link
Member

I still think it's a mistake to leave fixing the other issues identified in #29 (comment) to a future 0.5, since it will mean that we have to write code that upconverts 0.3 to both 0.4 and to 0.5, and 0.4 to 0.5 (where PME would be renamed to more accurately reflect the fact that this tag has nothing to do with PME but in fact indicates Ewald with a specified boundary condition is the correct reference method).

Is the plan to immediately propose the 0.5 version with the appropriate naming convention changes (e.g. PME -> Ewald3D-ConductingBoundary) and keyword changes (method -> periodic_potential), along with the extensions to specify specific reaction field functional forms and keywords? What's the plan here?

@davidlmobley
Copy link

@jchodera I think Matt is open to alternate proposals, but unless there ARE alternate proposals, we have to move forward with what we have, since we clearly need to resolve the present ambiguity. What do you propose instead?

@jchodera jchodera mentioned this pull request Mar 16, 2022
@jchodera
Copy link
Member

What do you propose instead?

#34

@mattwthompson
Copy link
Member Author

It appears #34 has the momentum (which I am excited about!) so I'll close this now. Can revisit if anything changes.

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.

SMIRNOFF: PME electrostatics cannot be applied to non-periodic systems
4 participants