Loading a ForceField from file substitutes units inappropriately

[lilyminium]

Describe the bug

Loading a ForceField from file substitutes units inappropriately. For example, "F" seems to get converted into 1 * farad, and K to 1 * kelvin.

To Reproduce

The easiest way to see this is to load base Sage:

>>> import openff.toolkit
>>> openff.toolkit.__version__
'0.11.4'
>>> from openff.toolkit import ForceField
>>> ff = ForceField("openff-2.0.0.offxml")
>>> charge_handler = ff.get_parameter_handler("LibraryCharges")
>>> charge_handler.parameters[2]
<LibraryChargeType with smirks: [#19+1:1]  charge1: 1.0 elementary_charge  name: 1 kelvin  >

Output

Checking https://github.com/openforcefield/openff-forcefields/blob/e2cb3b0746a173eb18d499bc680d9758ccaae657/openforcefields/offxml/openff-2.0.0.offxml#L363 shows the original name was just K+.

Computing environment (please complete the following information):

• Operating system
• Output of running conda list
• toolkit version: 0.11.4

Additional context

The example given here isn't too bad. However, it's a tad inconvenient because it means users can't use element symbol smarts (.to_smiles) but have to use atomic numbers instead (have to pass through RDKit's Chem.MolToSmarts). It's also surprising, as I think SMIRKS allows you to use symbols as element specifications. However, parameters like the LibraryCharge below get interpreted as an unholy mishmash of coulomb and farad.

<LibraryCharge smirks="[C:1](=[C:2]([F:3])[F:4])([C:5]([F:6])([F:7])[F:8])[F:9]" charge1="-0.053769998252391815 * elementary_charge ** 1" charge2="0.3786599934101105 * elementary_charge ** 1" charge3="-0.12859000265598297 * elementary_charge ** 1" charge4="-0.12859000265598297 * elementary_charge ** 1" charge5="0.69309002161026 * elementary_charge ** 1" charge6="-0.21591000258922577 * elementary_charge ** 1" charge7="-0.21591000258922577 * elementary_charge ** 1" charge8="-0.21591000258922577 * elementary_charge ** 1" charge9="-0.11307000368833542 * elementary_charge ** 1"></LibraryCharge>

And an aliphatic smarts would be interpreted as 1 ampere.

<LibraryCharge smirks="[A:1]" charge1="0 * elementary_charge ** 1" id="any"></LibraryCharge>

Loading a file that contains the above gets me:

---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
File ~/anaconda3/envs/openff-toolkit-release/lib/python3.9/site-packages/openff/toolkit/typing/engines/smirnoff/parameters.py:430, in ParameterAttribute._call_converter(self, value, instance)
    428 try:
    429     # Static function.
--> 430     return self._converter(value)
    431 except TypeError:
    432     # Instance method.

TypeError: smirks() missing 2 required positional arguments: 'attr' and 'smirks'

During handling of the above exception, another exception occurred:

TypeError                                 Traceback (most recent call last)
Cell In[7], line 1
----> 1 ForceField("forcefield.offxml")

File ~/anaconda3/envs/openff-toolkit-release/lib/python3.9/site-packages/openff/toolkit/typing/engines/smirnoff/forcefield.py:308, in ForceField.__init__(self, aromaticity_model, parameter_handler_classes, parameter_io_handler_classes, disable_version_check, allow_cosmetic_attributes, load_plugins, *sources)
    305 self._register_parameter_io_handler_classes(parameter_io_handler_classes)
    307 # Parse all sources containing SMIRNOFF parameter definitions
--> 308 self.parse_sources(sources, allow_cosmetic_attributes=allow_cosmetic_attributes)

File ~/anaconda3/envs/openff-toolkit-release/lib/python3.9/site-packages/openff/toolkit/typing/engines/smirnoff/forcefield.py:769, in ForceField.parse_sources(self, sources, allow_cosmetic_attributes)
    767 for source in sources:
    768     smirnoff_data = self.parse_smirnoff_from_source(source)
--> 769     self._load_smirnoff_data(
    770         smirnoff_data, allow_cosmetic_attributes=allow_cosmetic_attributes
    771     )

File ~/anaconda3/envs/openff-toolkit-release/lib/python3.9/site-packages/openff/toolkit/typing/engines/smirnoff/forcefield.py:929, in ForceField._load_smirnoff_data(self, smirnoff_data, allow_cosmetic_attributes)
    922 # Retrieve or create parameter handler, passing in section_dict to check for
    923 # compatibility if a handler for this parameter name already exists
    924 handler = self.get_parameter_handler(
    925     parameter_name,
    926     section_dict,
    927     allow_cosmetic_attributes=allow_cosmetic_attributes,
    928 )
--> 929 handler._add_parameters(
    930     parameter_list_dict, allow_cosmetic_attributes=allow_cosmetic_attributes
    931 )

File ~/anaconda3/envs/openff-toolkit-release/lib/python3.9/site-packages/openff/toolkit/typing/engines/smirnoff/parameters.py:1919, in ParameterHandler._add_parameters(self, section_dict, allow_cosmetic_attributes)
   1916 for unitless_param_dict in val:
   1918     param_dict = attach_units(unitless_param_dict, attached_units)
-> 1919     new_parameter = self._INFOTYPE(
   1920         **param_dict, allow_cosmetic_attributes=allow_cosmetic_attributes
   1921     )
   1922     self._parameters.append(new_parameter)

File ~/anaconda3/envs/openff-toolkit-release/lib/python3.9/site-packages/openff/toolkit/typing/engines/smirnoff/parameters.py:3125, in LibraryChargeHandler.LibraryChargeType.__init__(self, **kwargs)
   3124 def __init__(self, **kwargs):
-> 3125     super().__init__(**kwargs)
   3126     unique_tags, connectivity = GLOBAL_TOOLKIT_REGISTRY.call(
   3127         "get_tagged_smarts_connectivity", self.smirks
   3128     )
   3129     if len(self.charge) != len(unique_tags):

File ~/anaconda3/envs/openff-toolkit-release/lib/python3.9/site-packages/openff/toolkit/typing/engines/smirnoff/parameters.py:1769, in ParameterType.__init__(self, smirks, allow_cosmetic_attributes, **kwargs)
   1767 # This is just to make smirks a required positional argument.
   1768 kwargs["smirks"] = smirks
-> 1769 super().__init__(allow_cosmetic_attributes=allow_cosmetic_attributes, **kwargs)

File ~/anaconda3/envs/openff-toolkit-release/lib/python3.9/site-packages/openff/toolkit/typing/engines/smirnoff/parameters.py:860, in _ParameterAttributeHandler.__init__(self, allow_cosmetic_attributes, **kwargs)
    858 for key, val in smirnoff_data.items():
    859     if key in allowed_attributes:
--> 860         setattr(self, key, val)
    861     # Handle all unknown kwargs as cosmetic so we can write them back out
    862     elif allow_cosmetic_attributes:

File ~/anaconda3/envs/openff-toolkit-release/lib/python3.9/site-packages/openff/toolkit/typing/engines/smirnoff/parameters.py:1175, in _ParameterAttributeHandler.__setattr__(self, key, value)
   1170         raise MissingIndexedAttributeError(
   1171             f"'{key}' is out of bounds for indexed attribute '{attr_name}'"
   1172         )
   1174 # Forward the request to the next class in the MRO.
-> 1175 super().__setattr__(key, value)

File ~/anaconda3/envs/openff-toolkit-release/lib/python3.9/site-packages/openff/toolkit/typing/engines/smirnoff/parameters.py:379, in ParameterAttribute.__set__(self, instance, value)
    377 def __set__(self, instance, value):
    378     # Convert and validate the value.
--> 379     value = self._convert_and_validate(instance, value)
    380     setattr(instance, self._name, value)

File ~/anaconda3/envs/openff-toolkit-release/lib/python3.9/site-packages/openff/toolkit/typing/engines/smirnoff/parameters.py:397, in ParameterAttribute._convert_and_validate(self, instance, value)
    395 value = self._validate_units(value)
    396 # Call the custom converter before setting the value.
--> 397 value = self._call_converter(value, instance)
    398 return value

File ~/anaconda3/envs/openff-toolkit-release/lib/python3.9/site-packages/openff/toolkit/typing/engines/smirnoff/parameters.py:433, in ParameterAttribute._call_converter(self, value, instance)
    430         return self._converter(value)
    431     except TypeError:
    432         # Instance method.
--> 433         return self._converter(instance, self, value)
    434 return value

File ~/anaconda3/envs/openff-toolkit-release/lib/python3.9/site-packages/openff/toolkit/typing/engines/smirnoff/parameters.py:1750, in ParameterType.smirks(self, attr, smirks)
   1742 @smirks.converter
   1743 def smirks(self, attr, smirks):
   1744     # Validate the SMIRKS string to ensure it matches the expected
   (...)
   1748     # TODO: Add check to make sure we can't make tree non-hierarchical
   1749     #       This would require parameter type knows which ParameterList it belongs to
-> 1750     ChemicalEnvironment.validate_smirks(smirks, validate_valence_type=True)
   1751     return smirks

File ~/anaconda3/envs/openff-toolkit-release/lib/python3.9/site-packages/openff/toolkit/typing/chemistry/environment.py:152, in ChemicalEnvironment.validate_smirks(cls, smirks, validate_parsable, validate_valence_type, toolkit_registry)
    118 @classmethod
    119 def validate_smirks(
    120     cls,
   (...)
    124     toolkit_registry=None,
    125 ):
    126     """
    127     Check the provided SMIRKS string is valid, and if requested, tags atoms appropriate to the
    128     specified valence type.
   (...)
    150         and validate_valence_type=True
    151     """
--> 152     cls(
    153         smirks,
    154         validate_parsable=validate_parsable,
    155         validate_valence_type=validate_valence_type,
    156         toolkit_registry=toolkit_registry,
    157     )

File ~/anaconda3/envs/openff-toolkit-release/lib/python3.9/site-packages/openff/toolkit/typing/chemistry/environment.py:84, in ChemicalEnvironment.__init__(self, smirks, label, validate_parsable, validate_valence_type, toolkit_registry)
     82 self.label = label
     83 if validate_parsable or validate_valence_type:
---> 84     self.validate(
     85         validate_valence_type=validate_valence_type,
     86         toolkit_registry=toolkit_registry,
     87     )

File ~/anaconda3/envs/openff-toolkit-release/lib/python3.9/site-packages/openff/toolkit/typing/chemistry/environment.py:110, in ChemicalEnvironment.validate(self, validate_valence_type, toolkit_registry)
     89 def validate(self, validate_valence_type=True, toolkit_registry=None):
     90     """
     91     Returns True if the underlying smirks is the correct valence type, False otherwise. If the expected type
     92     is None, this method always returns True.
   (...)
    108         and validate_valence_type=True
    109     """
--> 110     perceived_type = self.get_type(toolkit_registry=toolkit_registry)
    111     if validate_valence_type and self._expected_type is not None:
    112         if perceived_type != self._expected_type:

File ~/anaconda3/envs/openff-toolkit-release/lib/python3.9/site-packages/openff/toolkit/typing/chemistry/environment.py:189, in ChemicalEnvironment.get_type(self, toolkit_registry)
    185     unique_tags, conn = toolkit_registry.get_tagged_smarts_connectivity(
    186         self.smirks
    187     )
    188 else:
--> 189     unique_tags, conn = toolkit_registry.call(
    190         "get_tagged_smarts_connectivity", self.smirks
    191     )
    193 if unique_tags == (1,):
    194     if len(conn) == 0:

File ~/anaconda3/envs/openff-toolkit-release/lib/python3.9/site-packages/openff/toolkit/utils/toolkit_registry.py:356, in ToolkitRegistry.call(self, method_name, raise_exception_types, *args, **kwargs)
    354             for exception_type in raise_exception_types:
    355                 if isinstance(e, exception_type):
--> 356                     raise e
    357             errors.append((toolkit, e))
    359 # No toolkit was found to provide the requested capability
    360 # TODO: Can we help developers by providing a check for typos in expected method names?

File ~/anaconda3/envs/openff-toolkit-release/lib/python3.9/site-packages/openff/toolkit/utils/toolkit_registry.py:352, in ToolkitRegistry.call(self, method_name, raise_exception_types, *args, **kwargs)
    350 method = getattr(toolkit, method_name)
    351 try:
--> 352     return method(*args, **kwargs)
    353 except Exception as e:
    354     for exception_type in raise_exception_types:

File ~/anaconda3/envs/openff-toolkit-release/lib/python3.9/site-packages/openff/toolkit/utils/openeye_wrapper.py:2652, in OpenEyeToolkitWrapper.get_tagged_smarts_connectivity(self, smarts)
   2649 from openff.toolkit.typing.chemistry import SMIRKSParsingError
   2651 qmol = oechem.OEQMol()
-> 2652 status = oechem.OEParseSmarts(qmol, smarts)
   2653 if not status:
   2654     raise SMIRKSParsingError(
   2655         f"OpenEye Toolkit was unable to parse SMIRKS {smarts}"
   2656     )

File ~/anaconda3/envs/openff-toolkit-release/lib/python3.9/site-packages/openeye/oechem.py:27797, in OEParseSmarts(*args)
  27792 def OEParseSmarts(*args) -> "bool":
  27793     r"""
  27794     OEParseSmarts(OEQMolBase arg1, char const * arg2, unsigned int opt=) -> bool
  27795     OEParseSmarts(OEQMolBase arg1, char const * arg2, OEVectorBindings vb, unsigned int opt=) -> bool
  27796     """
> 27797     return _oechem.OEParseSmarts(*args)

TypeError: Wrong number or type of arguments for overloaded function 'OEParseSmarts'.
  Possible C/C++ prototypes are:
    OEChem::OEParseSmarts(OEChem::OEQMolBase &,char const *,unsigned int)
    OEChem::OEParseSmarts(OEChem::OEQMolBase &,char const *)
    OEChem::OEParseSmarts(OEChem::OEQMolBase &,char const *,OEChem::OEVectorBindings const &,unsigned int)
    OEChem::OEParseSmarts(OEChem::OEQMolBase &,char const *,OEChem::OEVectorBindings const &)

One solution could be to restrict this parameter interpretation to not apply to known keywords, like smirks and name.

[mattwthompson]

I ran with your suggestion in #1494 and it seemed to work quite well. Have you come across other keys that trigger this, or just ["smirks", "name"]?

[mattwthompson]

Resolved for now in the linked PR, which should land in 0.11.5 or 0.12.0 this month or next.

This comment includes useful details for a longer-term fix: #1494 (comment)