Openff-toolkit: Where did sigma go in parameter handlers?

[proteneer]

Describe the bug

We're on a rather old version of the off toolkit (May 2019), and we're currently parsing the parameter handlers using:

if handler_name == "vdW":
            ...
            for param in handler_params.parameters:
                sigma = to_md_units(param.sigma)

For some reason when we updated to the current version, param.sigma, although defined, now defaults to None.

A clear and concise description of what the bug is.

To Reproduce

Access the param.sigma field

Output

print(param.sigma) shows None

Computing environment (please complete the following information):

• Linux
• Not using conda

Additional context

I think at some point we defaulted to rmin_half as opposed to sigma, and it's only populated using some magical incantation.

[jchodera]

We should definitely allow both retrieval and setting of vdW parameters by _either_ sigma or the less commonly used (but occasionally convenient)rmin_half!

[jchodera]

To be clear: Both sigma and rmin_half are supposed to be properties that can be set/get for vdW parameters. The internal representation that was originally used to define the parameter is remembered so that we don't accidentally read rmin_half and write out sigma, but we _must_ provide convenience accessors to change or set either.

[j-wags]

Ah, I see. It looks like we used to convert everything to be sigma (even if it was input as rmin_half): https://github.com/openforcefield/openforcefield/blob/0.2.0/openforcefield/typing/engines/smirnoff/parameters.py#L1402-L1405

Now, we more strictly respect the original parameter, which is rmin_half for both smirnoff99Frosst and the Parsley line.

we must provide convenience accessors to change or set either.

I agree that we should do this. I'll add @propertys for getting and setting both vdW parameters (sigma and rmin_half), with the getters doing conversion in the background if needed. Is there any clear desired behavior for which should be written out? That is, if a user loads Parsley and sets a sigma, should that parameter be written out with an rmin_half or sigma?

I'm in favor of writing out the most recently-set representation (so, in the above example, sigma should be written out)

[jchodera]

Now, we more strictly respect the original parameter, which is rmin_half for both smirnoff99Frosst and the Parsley line.

Note that the spec clearly states that _either_ sigma and rmin_half can be specified. Just like with units, we should (1) allow either to be get/set, (2) remember which one we set most recently, and (3) provide a nice way to write out parameters uniformly (e.g. tell it to write out everything in sigma) to make things nice on output if desired.

[j-wags]

Hm, I can do (1) and (2) very easily, and have it merged in time for the 0.7.0 release. (3) is tricky since it'll thread a new kwarg through, and I'll need to think hard if it will break encapsulation in a way that will prevent us from switching to a pydantic object model in the future.

[jchodera]

(3) is tricky since it'll thread a new kwarg through, and I'll need to think hard if it will break encapsulation in a way that will prevent us from switching to a pydantic object model in the future.

I think we can re-imagine this feature and implement it later. Essentially, we just need some way to say "give me a nicely homogenized ForceField object to serialize", so even methods that go through and homogenize things according to preferred units and parameter representations would be a fine way to do this.

[mattwthompson]

Looks like this slipped through the cracks:

In [23]: param
Out[23]: <vdWType with smirks: [#6X4:1]  epsilon: 0.1094 kcal/mol  id: n16  rmin_half: 1.908 A  >

In [24]: param.rmin_half
Out[24]: Quantity(value=1.908, unit=angstrom)

In [25]: param.sigma  # (nothing)

I have a partially-done implementation in a branch that I'll revisit.

[jchodera]

Would be great to have this functional! rmin_half is 🤮