Clarify that `long_range_dispersion` is implicit in vdW section version 0.3

[mikemhenry]

I noticed that the text describing the vdw mentions long_range_dispersion, default: "isotropic" but is missing from the table.

[j-wags]

This is complicated because of the following:

• The SMIRNOFF spec text for the vdW section currently only mentions long_range_disperson
• The SMIRNOFF spec table for the vdW section currently only mentions method
• Every mainline force field only contains a value for method
• The vdWHandler in the OpenFF Toolkit only contains an attribute for method

I've been meaning to look into this (it's been on my to-do list for years), but it seems possible that the mention of long_range_dispersion in the SMIRNOFF text is a typo, and the same information is now contained in the method field. Though I'm not an expert in this stuff so I can't tell if the method field (with allowed values {'cutoff', 'PME'}) contains the same information as long_range_dispersion (allowed value isotropic). Do either of you know if these are distinct fields?

[mattwthompson]

Do either of you know if these are distinct fields?

Long-range dispersion treatment and cutoff method are separate settings (and also separate from whether or not a switching function is used). You can have a cutoff with no long-range dispersion or a cutoff with long-range dispersion. It does not make sense to have long-range dispersion corrections with PME or a method that attempts to cover all long-range forces.

In OpenMM, long-range dispersion corrects are set via NonbondedForce.setUseDispersionCorrection(). In the toolkit's hot code path, this is set to True and presumably has been for years. Interchange does the same thing in its analogous path. There are similar settings in GROMACS, LAMMPS, and I bet Amber as well. There may be other ways of doing it than isotropic but I'm not well-read on those. Every point of our infrastructure has been using long_range_dispersion="isotropic" as the default behavior and likely always will; if anything I'd advocate that our implementation is incorrect and there should be a vdWHandler.long_range_dispersion field (which I would only implement "isotropic" of.) I would be happy to submit this patch.

This setting matters for accurately reproducing condensed-phase energies and it needs to be in the force fields if we wan to accurately reproduce them. I believe this is the paper I skimmed on the topic some time ago; I don't recall how much of a difference it makes but the community in general turns it on and doesn't look back.

[mikemhenry]

I'm not sure, I'm going over the spec with a fine tooth comb and building a pydatnic data model from the tag attributes and just noticed the discrepancy. I do know that both PME and vdW both can have long range dispersion corrections, so it may need to be something we think about for both of them.

[j-wags]

Ok, @mattwthompson convinced me that these are distinct.

Unfortunately, this is a breaking change. A parser that reads a file after this spec change will fill in the default value long_range_dispersion='isotropic', and will then write it out in a line <vdW version=0.3 ... long_range_dispersion='isotropic'>. Then an older reader could see this section, assume it's safe to read ("I can read vdW section version 0.3") and then fail because of the new keyword.

So to add the keyword, I think this would need to bump the vdW section version to 0.4. And a section version bump would need an EP. If desired the EP could do the same for the Electrostatics tag. But an entire EP is a lot of lifting.

As an alternative, what if we left long_range_dispersion out of the table, and updated the text to say "vdW section version 0.3 assumes that the long-range dispersion handling is isotropic". I think that would be a clear typo fix and I'd approve that outside the EP process.

[mattwthompson]

I'd be in support of making the seemingly-obvious improvement here as implementing the up-converter in the toolkit seems trivial at first glance. However, we can just back-map the current implementation into the specification in a way that one can claim is not a breaking change - but also totally defers the question of whether or not it should actually be a setting in the future.

If we want SMIRNOFF to be flexible to do stuff outside of what our mainline force fields do, then we're going to need to pay that cost at some point, and it's generally more painful in the distant future than the near future. If that's not a priority, then we should focus on what amounts to a single set of re-usable non-bonded parameters and not worry ourselves with expanding the scope from there. I wouldn't be a fan of that, since it pushes the cruft of non-bonded settings downstream to Interchange and user support, but those preferences need not be shared by everybody.

I've actually wanted this change for a while for my own uses (which are not important to anybody else as far as I'm aware). I obfuscated the original PR #22 with a mountain of confusion but that could be totally re-done with the above proposal ("add the keyword, bump to 0.4") and also listing long_range_dispersion="none" as a (non-default) option.

[mattwthompson]

Thanks @j-wags! I am retracting my approval while we figure out the preferred way forward. I would be in favor of either option implied in this comment:

1. Modify this EP to be the full version bump as described and accept the breaking changes [^1]. Included should be a recommendation of auto-up-conversion behavior (like [here], but much smaller in scope). I'd recommend the behavior be "up-converting from 0.3 to 0.4 assumes the default value of long_range_dispersion="isotropic"). This would require approval of the SMIRNOFF committee, whoever that currently includes.
2. Only modify the text ("updated the text to say 'vdW section version 0.3 assumes that the long-range dispersion handling is isotropic'") now and implement a full version bump to 0.4 in a separate EP, either immediately or pushed off into the future. This change alone would not, in my judgement, require review through the committee for the reasons Jeff described above.

Would you like to continue to move this forward @mikemhenry? If so, please pick either path - and if not I can take this on myself.

[^1]: The effects on downstream developers should be minimal; the patches required to update the toolkit and Interchange really should be small, and I/we can take responsibility for those. The new structure of a round-tripped XML might be a surprise to some users, but given that we are discussing this out in the open and our tools can be expected to parse the new versions, I do not think this is an unreasonable burden to users.

[mikemhenry]

I think path 2 is the best, since there may be other changes that we want to do in the 0.4 bump. I will modify this PR accordingly.

implement a full version bump to 0.4 in a separate EP, either immediately or pushed off into the future

What do we need to do to make sure this doesn't get forgotten about? Really, my only reservation with path 2 is that we will forget about it.

[mikemhenry]

@mattwthompson let me know if that was what you were thinking RE text wise, feel free to modify it if you think it should go somewhere else or the phrasing could be better.

[mattwthompson]

What do we need to do to make sure this doesn't get forgotten about?

I'll add it to our board and try to get to it next week. You are welcome to take a stab at if you need it in shorter time, but our data and stack assume isotropic long-range dispersion from top to bottom, so you could safely assume that in your tool today.

[mikemhenry]

@j-wags @mattwthompson I know this is a toolkit thing but RE our conversation on this PR:

>>> force_field = ForceField("openff-2.0.0.offxml")
>>> force_field.get_parameter_handler('vdW').long_range_dispersion
Traceback (most recent call last):
  File "/home/mmh/miniconda3/envs/pydantic-fun-times/lib/python3.9/site-packages/openff/toolkit/typing/engines/smirnoff/parameters.py", line 1149, in __getattr__
    return super().__getattr__(item)
AttributeError: 'super' object has no attribute '__getattr__'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/mmh/miniconda3/envs/pydantic-fun-times/lib/python3.9/site-packages/openff/toolkit/typing/engines/smirnoff/parameters.py", line 1154, in __getattr__
    raise AttributeError(
AttributeError: <class 'openff.toolkit.typing.engines.smirnoff.parameters.vdWHandler'> object has no attribute 'long_range_dispersion'
>>> 

Should work and look like:

>>> force_field.get_parameter_handler('vdW').long_range_dispersion
'isotropic'

[j-wags]

In the spec's current state (post merge), I actually don't think it should work like that - The vdWHandler shouldn't volunteer information that's outside the information content of the spec. We can interpret other "unvolunteered" information as precedent, like PME error tolerance (which is set to something in the systems we make, but it's not documented in the spec so it's just set to the OpenMM default)