search

openforcefield / standards

A repository of the standards employed across the Open Force Field Consortium.
https://openforcefield.github.io/standards
MIT License
1 stars 3 forks source link

OFF-EP 0007b — Add long-range vdW treatment attribute in vdW section (alternative to OFF-EP 0007) #44

Open jchodera opened 1 year ago

jchodera commented 1 year ago

This is an alternative to #40, cleanly clarifying all the concerns I raised in https://github.com/openforcefield/standards/pull/40#issuecomment-1191771124.

It may be easiest to review the changes with respect to #40 here: https://github.com/openforcefield/standards/pull/44/commits/507b579e4f7ddf4f656c9c08bff68fe97ef2b299

davidlmobley commented 1 year ago

I'll review, but one thing that makes me nervous about this is that -- unless I'm misunderstanding -- approving this would require us to immediately add LJPME support (and tests etc), and our developers already have a backlog relating to biopolymer support. Am I missing something?

jchodera commented 1 year ago

Could we just tag the LJMPE option with a "not yet implemented" note? We can just remove that without OFF-EP review once it is implemented in the toolkit.

In terms of how much work it will be to implement: If we are already splitting electrostatics and vdW into separate OpenMM Forces, this will be very easy to implement. If we are not yet doing this, we will probably want to do that as part of a refactoring when we implement this. OpenMM performance impact should be minimal.

j-wags commented 1 year ago

This EP is looking pretty good, though I could use clarification on why we'd use method='LJPME' long_range_correction='LJPME' as opposed to method='PME' long_range_correction='none'. My current (very limited!) understanding is that the long- and short-distance parts of PME are inseparable.

Also, is LJPME important to distinguish from PME? I've only ever seen it in the context of OpenMM's API so I had assumed LJPME was just a unique string chosen for the nonbonded namespace (where PME has a distinct meaning).

jchodera commented 1 year ago

@mrshirts: Could you review this when you have a chance?

mrshirts commented 1 year ago

Added some comments - some minor clarification points (some relating to other aspects of long-range LJ corrections), and an explicit question about how we support two options in a spec that can give statistically different results.