Openff-toolkit: Cannot add a new bond parameter via OFFXML; fails with IncompatibleParameterError: fractional_bondorder_method values are not identical

[trevorgokey]

Describe the bug
Running tests for vsites, and I am explicitly adding a virtual site handler, and since I need a dummy bond of O=O, I supply the following:

xml_ff_virtual_sites_bondcharge_match_once = """<?xml version="1.0" encoding="utf-8"?>
<SMIRNOFF version="0.3" aromaticity_model="OEAroModel_MDL">
    <Bonds version="0.3">
      <Bond smirks="[*:1]~[*:2]" id="b999" k="500.0 * kilocalories_per_mole/angstrom**2" length="1.1 * angstrom"/>
    </Bonds>
    <VirtualSites version="0.3">
        <VirtualSite
            type="BondCharge"
            name="EP"
            smirks="[*:1]~[*:2]"
            distance="0.1*angstrom"
            chargeincrement1="0.1*elementary_charge"
            chargeincrement2="0.1*elementary_charge"
            sigma="0.1*angstrom"
            epsilon="0.1*kilocalories_per_mole"
            match="once" >
        </VirtualSite>
        <VirtualSite
            type="BondCharge"
            name="EP"
            smirks="[#7:1]~[#7:2]"
            distance="0.2*angstrom"
            chargeincrement1="0.2*elementary_charge"
            chargeincrement2="0.2*elementary_charge"
            sigma="0.2*angstrom"
            epsilon="0.2*kilocalories_per_mole"
            match="all_permutations" >
        </VirtualSite>
        <VirtualSite
            type="BondCharge"
            name="EP"
            smirks="[#7:1]~[#7:2]"
            distance="0.2*nanometers"
            chargeincrement1="0.2*elementary_charge"
            chargeincrement2="0.2*elementary_charge"
            sigma="0.2*angstrom"
            epsilon="0.2*kilocalories_per_mole"
            match="once" >
        </VirtualSite>
    </VirtualSites>
</SMIRNOFF>
"""

where I am specifically trying to test a 2-atom molecule with something other than dinitrogen. I get the following error:

_______________________________________________ TestForceFieldVirtualSites.test_bond_charge_virtual_site_parameters[args3-toolkit_registry0-OE] _______________________________________________
[gw8] linux -- Python 3.7.8 /home/tgokey/.local/miniconda3/envs/oFF/bin/python

self = <openforcefield.tests.test_forcefield.TestForceFieldVirtualSites object at 0x7fa52fdc63d0>, toolkit_registry = ToolkitRegistry containing OpenEye Toolkit, registry_description = 'OE'
args = OrderedDict([('xml', '<?xml version="1.0" encoding="utf-8"?>\n    <SMIRNOFF version="0.3" aromaticity_model="OEAroMode...e=0.2, unit=angstrom), Quantity(value=0.2, unit=kilocalorie/mole)))), ('mol', Molecule with name '' and SMILES 'N#N')])

    @pytest.mark.parametrize(
        "toolkit_registry,registry_description", toolkit_registries
    )
    @pytest.mark.parametrize("args", bond_charge_parameters_args)
    def test_bond_charge_virtual_site_parameters(
        self, toolkit_registry, registry_description, args
    ):
        """
        Test force fields with bond charge lone pair virtual sites
        """

>       self._test_physical_parameters(toolkit_registry, *args.values())

openforcefield/tests/test_forcefield.py:2108: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
openforcefield/tests/test_forcefield.py:1931: in _test_physical_parameters
    forcefield = ForceField("openff-1.2.0.offxml", xml)
openforcefield/typing/engines/smirnoff/forcefield.py:357: in __init__
    self.parse_sources(sources, allow_cosmetic_attributes=allow_cosmetic_attributes)
openforcefield/typing/engines/smirnoff/forcefield.py:882: in parse_sources
    smirnoff_data, allow_cosmetic_attributes=allow_cosmetic_attributes
openforcefield/typing/engines/smirnoff/forcefield.py:1038: in _load_smirnoff_data
    allow_cosmetic_attributes=allow_cosmetic_attributes,
openforcefield/typing/engines/smirnoff/forcefield.py:792: in get_parameter_handler
    old_handler.check_handler_compatibility(new_handler)
openforcefield/typing/engines/smirnoff/parameters.py:2343: in check_handler_compatibility
    other_handler, identical_attrs=string_attrs_to_compare
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

self = <openforcefield.typing.engines.smirnoff.parameters.BondHandler object at 0x7fa52ff2eb50>
other = <openforcefield.typing.engines.smirnoff.parameters.BondHandler object at 0x7fa52fe2a310>
identical_attrs = ['potential', 'fractional_bondorder_method', 'fractional_bondorder_interpolation'], tolerance_attrs = (), tolerance = 1e-06

    def _check_attributes_are_equal(
        self, other, identical_attrs=(), tolerance_attrs=(), tolerance=1e-6
    ):
        """Utility function to check that the given attributes of the two handlers are equal.

        Parameters
        ----------
        identical_attrs : List[str]
            Names of the parameters that must be checked with the equality operator.
        tolerance_attrs : List[str]
            Names of the parameters that must be equal up to a tolerance.
        tolerance : float
            The absolute tolerance used to compare the parameters.
        """

        def get_unitless_values(attr):
            this_val = getattr(self, attr)
            other_val = getattr(other, attr)
            # Strip quantities of their units before comparison.
            try:
                u = this_val.unit
            except AttributeError:
                return this_val, other_val
            return this_val / u, other_val / u

        for attr in identical_attrs:
            this_val, other_val = get_unitless_values(attr)

            if this_val != other_val:
                raise IncompatibleParameterError(
                    "{} values are not identical. "
                    "(handler value: {}, incompatible value: {}".format(
>                       attr, this_val, other_val
                    )
                )
E               openforcefield.typing.engines.smirnoff.parameters.IncompatibleParameterError: fractional_bondorder_method values are not identical. (handler value: None, incompatible value: None

It seems like this is saying None != None, so I am not sure what to make of this. What is the intended behavior here? Did I not specify something in the BondHandler?

To Reproduce
Running pytest on test_forcefield.py on the vsite branch.

Additional context
Using a fresh master of the toolkit pulled this morning. The vsite branch is up to date with no conflicts.

[mattwthompson]

I'm not sure how this is best dealt with (it's an open question in one of my PRs #705) but the issue seems to be the classic "did you mean that to be None or "None", or, in our case, "is that part of the force field intentionally none, or just not specified?"

-> 2229             import ipdb; ipdb.set_trace()
   2230             if this_val != other_val:

ipdb> other_val
ipdb> this_val
'None'

I don't know of any recent changes to the master branch would cause this?

This could be reported in a more useful way since the default string-ifying doesn't draw the distinction:

In [1]: str(None)
Out[1]: 'None'

In [2]: str('None')
Out[2]: 'None'

It doesn't have to get added on to that PR but we should make that distinction in the traceback in order to close this issue.

For now, try modifying the <Bonds> line in each of your add-on XMLs to match what's probably in the test OFFXMLs (this is what greping for Bonds gets me):
```

[j-wags]

Oof, the ol' "None" vs. None problem. I was wondering when this would come up. Our current implementation is ambiguous.

Basically, we have three object representations we will need to support:

• "Python-land", where None and "None" are entirely different types
• "XML-land" where everything's a string (unless we use something like a null field, but I had seen some debate over whether this is kosher)
• "Pydantic-land", where I don't know how the None vs "None" conflict plays out.

Whichever strategy we choose needs to lead to unambiguous representation of any valid SMIRNOFF FF in each format, and it's a big plus if we don't abuse/overload fields in any above format.

I've been dragging my feet because I don't know how Pydantic does things. @mattwthompson -- can you comment on the situation with None values in pydantic?

[trevorgokey]

Thanks Matt!. Adding those default options got passed the error.

[mattwthompson]

It's pretty close to the Python representation in that it sees None and str("None") as completely different objects with nothing at all in common. (With some customization, it could be stored as either, if for some reason we wanted to intentionally add ambiguity into a model.) My preference would definitely be for things to be stored in Python-friendly representations long-term and deal with questions like these at relevant interface; I'd assume that Pydantic already deals with this when going to or from XMLs

@trevorgokey great! - I'd like to leave this open until that traceback is fixed, though, in order to prevent this from happening in the future

[trevorgokey]

Yes I agree, this bit could be improved. I think, coming from the non-implementer/user role, it would have been nice to have been told that I need to set fractional_bondorder_method="None" or something along those lines. The follow-up is clear from this discussion, but that error message contorted my face a bit :)

[mattwthompson]

I'd also add that this case also implies that certain cases of IncompatibleParameterError could be caught, i.e. if there are known patterns that would raise this error, an even more specific exception message could be used. The best solution here is probably inferring None from "None", but we could also do some error checking between if this_val != other_val: and raising the exception to handle speicif things like str("None")

[j-wags]

but we could also do some error checking between if this_val != other_val: and raising the exception to handle specific things like str("None")

I think you're right about this, Matt. At least for FFs, we're stuck with supporting a 'canonical' XML representation that uses the string "None". The best long-term decision here is probably actually to change the XMLParameterIOHandler to convert all appearances of the string "None" in an XML file to actual python NoneTypes. (Indeed, it seems I de-facto made this a standard last August, but just didn't roll it out to everywhere it should be)

Once in a blue moon this may mangle someone's workflow that uses FF cosmetic attributes to store the string "None" somewhere, but there's just really no good way to overload a format that can't distinguish between the two.