Interaction between preset charges/`charge_from_moleucles` and virtual sites underspecified

[mattwthompson]

Consider a case in which a water Molecule has arbitrary partial charges and is fed to a well-defined SMIRNOFF force field for 4-site water (in atoms have charges defined by a LibraryCharges and VirtualSite parameters have non-zero charge increments).

What's supposed to happen here?

1. Preset charges "win" and the virtual site just doesn't have a chance to get charges assigned
2. Preset charges are meant to replace library charges/AM1-BCC or similar and virtual site charge increments should still be applied.

(1) is at least counter-intuitive since charge increments in virtual site parameter shouldn't be optional or modified by preset charges. (2) doesn't match the description in "Prespecified charges (reference implementation only)" in the spec; the resulting partial charges differ from what's passed in.

I think the case of a ligand subject to both AM1-BCC charges and virtual site parameters raises the same question, in case that's easier to digest.

More detail: https://github.com/openforcefield/openff-interchange/issues/1050#issuecomment-2359486748

[lilyminium]

IMO 2) seems most appropriate to me in terms of practical implementation, usage flexibility, and what I expect from a force field with virtual sites (especially one whose sole purpose is to implement off-site charges). In terms of practical implementation, as pointed out in https://github.com/openforcefield/openff-interchange/issues/1050 that seems to be already what happens in Interchange. If users dislike that outcome, from a flexibility point of view it seems more feasible to remove or modify the vsite parameter in the force field than deal with charge-assignment behaviour deep in Interchange. And lastly if I use a force field with off-site charges I would be surprised to find them missing if I edited the charges of real atoms on my molecule; I haven't checked but I didn't think it was currently possible to easily pass in the vsite charges with charge_from_molecules. (On that -- as a user it might be useful to have a warning upon creating an Interchange with a FF with virtual sites and charge_from_molecules that vsites would have been parameterized with whatever charge method is in the FF!)

Which also brings me to a good real use-case -- all my current vsite FFs use a ChargeModelIncrementHandler with a partial charge method of "am1elf10" since that's what I fit with. This of course is difficult for people without OpenEye to run -- I've had to manually edit that to "am1-mulliken" to use it with Ambertools. If users wanted to keep the am1elf10 method and simply pass in charges generated elsewhere with charge_from_molecules, being able to apply the virtual sites on top would be incredibly beneficial.

Basically this would be a strong vote for updating the spec to describe the current behaviour in more detail.

[j-wags]

I agree with everything @lilyminium said in the first paragraph:

• 2 is the desired behavior
• if we're concerned about user surprise then we should be emitting warnings. It'd be good to carefully think through these cases though, since casual users plug-and-playing published OFFXMLs should get warnings before they're surprised, but if people are editing OFFXML files it will be extremely difficult for us to imagine all the things that might surprise them.

I see Lily's second paragraph as perhaps being most easily resolved by adding a new charge method like TryAm1Elf10ExceptAm1 - this would be unsightly but I imagine it'd only be accessed by expert users/FF creators, since once such experiments enough showed promise to be featured in a FF release, then the underlying charges could be provided by a NAGL model trained to am1elf10.

[davidlmobley]

I agree with Lily also.

The only thing I would add is that I think "what's the desired behavior" is less important than "is the spec clear"; anyone futzing with these things is doing development/bleeding edge stuff and they are going to have to consult the spec/docs carefully, so what's important is that the spec and docs are clear, as they're likely NOT coming in with some predefined idea of how things ought to work.

[mattwthompson]

Downstream in implementation world, Lily's (2) above is what was previously happening by accident and is now encoded as intended behavior. I'll leave this open until the desired (documentation) improvements in the spec are done