mattwthompson
commented
2 years ago I screwed this one up pretty bad. It might be salvageable, but it'll take some crafting. The key error I made is not distinguishing the cut-off method (i.e. tail correction) and long-range dispersion as separate things. They're highly coupled in practice but in principle they are different things. The cut-off method (as I'm using it here, at least) deals with addressing the discontinuity of the vdW potential at the cutoff distance (r_cutoff in the diagram, NOT the entire blue region) and the long-range dispersion correction is something extra added on top. In the original Shirts 2007 paper, they used both a switching function and a long-range dispersion correction. An earlier version of this OFF-EP instead proposed adding <vdW ... method="no-cutoff"/> but I scrapped that at some point in time because ... I'm not sure why. I'm now convincing myself that is a better proposal and instead we should be hashing out the different combinations of settings that should be allowed and when each should be required.
Part of the confusion here is that the spec completely skips the question of how to deal with the cutoff generally - it simply assumes that the the vdW interactions are going to be cutoff at some distance corrected with a switching function. method="no-cutoff" might help here but it still skips over the question of how a cutoff should be corrected, if at all. As examples, I'd rather see things like
<vdW ... method="cutoff" tail_correction="switching" cutoff_distance="" switch_distance="" ... /><vdW ... method="cutoff" tail_correction="none" cutoff_distance="" ... /><vdW ... method="no-cutoff" tail_correction="none" ... /><vdW ... method="pme" ... />
More verbose, but explicit is better than implicit when details matter.
Anyway, that's exploding the scope. Separately from all of this, and something that does not resolve #7, we probably should have an option that simply turns off the dispersion. ="isotropic" simply translates to .setUseDispersionCorrection(True), something that other implementations may not want to do, so we should have a corresponding method that does not do anything. I will attempt to re-scope this proposal to just that and split out the other topics into other OFF-EPs with more sweeping changes.
Here's my understanding of vdW energies:
This diagram roughly captures my (imperfect) understanding. There are many other options not considered in the current OpenMM-centric view; I'll try to hold my tongue on these but I have to note that the current spec is in large part a backmapping of common OpenMM usage.
Would the energy value for the flat region be zero, or the energy value from the rightmost point in the switching region?
It should be zero when no long-range dispersion correction is applied. I think this is only a consequence of the fact that no other options that are currently supported should modify this. Long-term the answer might become "whatever the other options result in it being," along with some safeguards if certain combinations of settings are nonsense.
Which keyword would control this?
I don't think this is something that is exposed and I don't think there's a clean way to support it now. Eventually people may want to do something like a [shifted Lennard-Jones potential], but I'd rather leave that to when we're considering more potential values.
I also have to add that I think these questions are somewhat valid for the "isotropic" option; the spec currently fails to describe it at all:
Attributes in the
tag specify ..., as well as the distance at which a switching function is applied (switch_width, default: "1.0angstrom"), the cutoff (cutoff, default: "9.0angstroms"), and long-range dispersion treatment scheme (long_range_dispersion, default: "isotropic").
And that's it. If I didn't have OpenMM's boolean toggle for this, I certainly wouldn't know what is intended by this.
The EP mentions that different engines use different switching schemes. Which keyword indicates that we don't want a switching scheme (effectively switching_function(r) = 0)?
None, and once I revamp this EP to only consider the long-range dispersion it should be out of scope. This will require more significant change, something like * <vdW ... method="cutoff" tail_correction="none" cutoff_distance="" ... /> that I suggest above.
I'm inclined to think that our current (admittedly flawed) implementation has long_range_dispersion define the energies to the right of the switching region, and method adds the information for what happens inside the switching region. So I'm cautious about this because the justification for this EP aims to change a single one of these keywords (long_range_dispersion) to get different behavior in both regions.
My reading of the OpenMM docs and paper (equation (1)), which could be totally wrong, imply that the long-range dispersion correction is applied everywhere, not just the purple region. I think this is what's happening in the source code as well. So in principle - though the numbers are probably tiny - even the direct space energy is not simply a 12-6 potential when this is applied.
Nonetheless, I agree with your concerns. I think this justifies a bigger change as I discussed above.
SimonBoothroyd
NOTE: Much of the discussion in this thread is around an outdated version of this OFF-EP. Please consider on the current diff of files.
Resolves #7