OFF-EP 0007 — Add long-range dispersion attribute in vdW section#40
OFF-EP 0007 — Add long-range dispersion attribute in vdW section#40mattwthompson wants to merge 8 commits into
Conversation
j-wags
left a comment
There was a problem hiding this comment.
Thanks for the excellent writeup. This looks good to me!
davidlmobley
left a comment
There was a problem hiding this comment.
Looks good, thanks for getting this in place/removing the ambiguity.
|
I've just taken a pass through this. I'm definitely in favor of removing ambiguity, but this appears to be too simplified. For example, it is very likely we will want to use LJPME instead of a simple isotropic correction. In addition, |
|
Thanks for the feedback!
This seems to be where we'd all like things to end up, but until then we need to provide guidance on the non-LJPME approach. As I understand it, these are mutually exclusive choices; one could either non_bonded_force.setNonbondedMethod(openmm.NonbondedForce.PME)
non_bonded_force.setUseDispersionCorrection(True)or non_bonded_force.setNonbondedMethod(openmm.NonbondedForce.LJPME)If it's a matter of clarity, and my understanding is accurate, I'd be happy to add in a comment along the lines of "If LJ-PME is used,
I completely agree and I admit I don't know what it's supposed to mean. It's been in the spec for years and as far as I understand it, the downstream effect is simply to flip
Perhaps it would be simplest to ask if you recall what the intended meaning of |
|
I agree with @jchodera 's questions, but I think these have to be dealt with separately from this EP. In particular, right now the spec is ambiguous or makes an unstated assumption -- that we are using an isotropic long-range dispersion correction for LJ. The immediate "stop the bleeding" response to this should be to state this assumption, which is what this EP proposes to do, and I think it's the right path forward. John's questions here are good:
But... we can't address this by changing the spec because, as Matt notes, this is not even addressed in the OpenMM docs, and even if it were, our spec couldn't control the form of the isotropic correction. I propose: And then relating to LJPME:
Yes, but that's a separate issue from the change here, which is to remove the unstated assumption our implementation of the spec is currently making (that it should be using an isotropic LJ dispersion correction). So any changes in that regard should come in a separate EP. I agree with @mattwthompson 's proposed solution:
|
non_bonded_force.setNonbondedMethod(openmm.NonbondedForce.PME)
non_bonded_force.setUseDispersionCorrection(True)
non_bonded_force.setNonbondedMethod(openmm.NonbondedForce.LJPME)Let's not make the mistake of confusing the actual logical choices here with how OpenMM chooses to implement things via its API (especially the convoluted I propose the following:
where |
|
Thanks for all of the feedback. I have implemented the two changes suggested by @davidlmobley above and also fixed a few typos. At this point I consider task I set out to do - contribute to the specification a detail that has been implicit for quite some period of time but in a way that enables future extensions - as accomplished. I advocate that further improvements should be handled in future EPs brought forth by members of the committee or the community more broadly. I understand that means this EP may not be accepted. |
|
@jchodera I propose we approve/merge this EP and migrate your suggestions to a separate EP. I don't yet understand (nor does @mattwthompson , I think) how to translate them to a specific change to the spec, which to me is a strong argument to making that a SEPARATE change to the spec. This EP fixes the problem it was intended to fix, in my view; the fact that there are other, related problems doesn't mean they MUST be dealt with in the same EP. |
@davidlmobley : I've clarified my points in an alternative EP: #44 #44 should provide a cleaner path forward that also allows us to start doing force field science in cases where LJPME is important for accurately modeling heterogeneous systems like membrane proteins. |
Implements a proposal as discussed recently; follow links in the EP for more information.
Supersedes #22
cc: @mikemhenry