Serialization/conversion performance improvements

.github/workflows/CI.yml

@@ -154,7 +154,7 @@ jobs:
           # TODO: Flip back to schedule condition before merge
           PYTEST_ARGS+=" --runslow"
 
-          python -m pytest -x --durations=20 $PYTEST_ARGS $COV openff/toolkit/_tests
+          python -m pytest --durations=20 $PYTEST_ARGS $COV openff/toolkit/_tests
 
       - name: Run code snippets in docs
         if: ${{ matrix.rdkit == true && matrix.openeye == true }}

openff/toolkit/_tests/test_parameters.py

@@ -822,6 +822,53 @@ class MyParameterHandler(ParameterHandler):
         ):
             handler.create_force()
 
+    def test_hash(self):
+        bh = BondHandler(skip_version_check=True, allow_cosmetic_attributes=True)
+
+        hash1 = hash(bh)
+        # Ensure hash changes when a new parameter is added
+        bh.add_parameter(
+            {
+                "smirks": "[*:1]-[*:2]",
+                "length": 1 * unit.angstrom,
+                "k": 10 * unit.kilocalorie / unit.mole / unit.angstrom ** 2,
+                "id": "b0",
+            }
+        )
+        hash2 = hash(bh)
+        assert hash1 != hash2
+
+        # Ensure hash changes when another parameter that differs only by SMIRKS is added
+        bh.add_parameter(
+            {
+                "smirks": "[C:1]-[C:2]",
+                "length": 1 * unit.angstrom,
+                "k": 10 * unit.kilocalorie / unit.mole / unit.angstrom ** 2,
+                "id": "b0",
+            }
+        )
+        hash3 = hash(bh)
+        assert hash2 != hash3
+
+        bh.add_cosmetic_attribute("fizz", "buzz")
+        hash3p5 = hash(bh)
+        assert hash3 != hash3p5
+
+        # Ensure hash changes when a cosmetic attribute is added
+        bh.parameters[0].add_cosmetic_attribute("foo", "bar")
+        hash4 = hash(bh)
+        assert hash3p5 != hash4
+
+        # Ensure hash changes when parameters are reordered
+        param = bh.parameters.pop(0)
+        bh.parameters.append(param)
+        hash5 = hash(bh)
+        assert hash4 != hash5
+
+        # Ensure hash doesn't change when the contents haven't changed
+        hash6 = hash(bh)
+        assert hash5 == hash6
+
 
 class TestParameterList:
     """Test capabilities of ParameterList for accessing and manipulating SMIRNOFF parameter definitions."""

openff/toolkit/typing/engines/smirnoff/forcefield.py

@@ -841,8 +841,7 @@ def _to_smirnoff_data(self, discard_cosmetic_attributes: bool = False) -> dict:
         for parameter_handler in self._parameter_handlers.values():
             # If _TAGNAME is None, the default value, an error should have been
             # thrown upon registering it, so assume it is str here
-            handler_tag: str = parameter_handler._TAGNAME  # type: ignore[assignment]
-            l1_dict[handler_tag] = parameter_handler.to_dict(
+            l1_dict[parameter_handler._TAGNAME] = parameter_handler.to_dict(
                 discard_cosmetic_attributes=discard_cosmetic_attributes
             )
 
@@ -1099,8 +1098,8 @@ def to_string(
         smirnoff_data = self._to_smirnoff_data(
             discard_cosmetic_attributes=discard_cosmetic_attributes
         )
-        string_data = io_handler.to_string(smirnoff_data)
-        return string_data
+
+        return io_handler.to_string(smirnoff_data)
 
     def to_file(
         self,
@@ -1432,13 +1431,13 @@ def __getitem__(self, val: Union[str, ParameterHandler]) -> ParameterHandler:
         Syntax sugar for lookikng up a ParameterHandler. Note that only
         string-based lookups are currently supported.
         """
-        if isinstance(val, str):
+        if type(val) is str:
             if val in self._parameter_handlers:
                 return self.get_parameter_handler(val)
             else:
                 raise KeyError(f"Parameter handler with name '{val}' not found.")
-        elif isinstance(val, ParameterHandler) or issubclass(val, ParameterHandler):
-            raise NotImplementedError
+        else:
+            raise NotImplementedError(f"Lookup by {type(val)=} is not supported.")
 
     def __hash__(self) -> int:
         """Deterministically hash a ForceField object

openff/toolkit/typing/engines/smirnoff/io.py

@@ -205,31 +205,34 @@ def prepend_all_keys(
                 A set or list of strings, indicating keys not to prepend in the data structure
 
             """
-            if isinstance(d, dict):
+            if type(d) is dict:
                 for key in list(d.keys()):
                     if key in ignore_keys:
                         continue
-                    if isinstance(d[key], list) or isinstance(d[key], dict):
+                    if type(d[key]) in (list, dict):
                         prepend_all_keys(d[key], char=char, ignore_keys=ignore_keys)
                     else:
                         new_key = char + key
                         d[new_key] = d[key]
                         del d[key]
                         prepend_all_keys(d[new_key], char=char, ignore_keys=ignore_keys)
-            elif isinstance(d, list):
+            elif type(d) is list:
                 for item in d:
                     prepend_all_keys(item, char=char, ignore_keys=ignore_keys)
 
         # the "xmltodict" library defaults to print out all element attributes on separate lines
         # unless they're prepended by "@"
         prepend_all_keys(smirnoff_data["SMIRNOFF"], ignore_keys=["Author", "Date"])
 
-        # Reorder parameter sections to put Author and Date at the top (this is the only
-        # way to change the order of items in a dict, as far as I can tell)
-        for key, value in list(smirnoff_data["SMIRNOFF"].items()):
-            if key in ["Author", "Date"]:
-                continue
-            del smirnoff_data["SMIRNOFF"][key]
-            smirnoff_data["SMIRNOFF"][key] = value
+        # Ensure Author and Date are listed first, necessitates creating a new dict
+        return_dict = {
+            "SMIRNOFF": {
+            "Author": smirnoff_data['SMIRNOFF'].pop("Author", "Author Unknown"),
+            "Date": smirnoff_data['SMIRNOFF'].pop("Date", "Date Unknown"),
+        }}
 
-        return xmltodict.unparse(smirnoff_data, pretty=True, indent=" " * 4)
+        return_dict['SMIRNOFF'].update(smirnoff_data['SMIRNOFF'])
+
+        del smirnoff_data
+
+        return xmltodict.unparse(return_dict, pretty=True, indent=" " * 4)

openff/toolkit/typing/engines/smirnoff/parameters.py

@@ -345,7 +345,7 @@ def __init__(
         converter: Optional[Callable] = None,
         docstring: str = "",
     ):
-        if isinstance(unit, str):
+        if type(unit) is str:
             # be careful with module & variable names
             unit = Unit(unit)
 
@@ -1086,37 +1086,38 @@ def to_dict(
     def __getattr__(self, item):
         """Take care of mapping indexed attributes to their respective list elements."""
 
-        # Try matching the case where there are two indices
-        # this indicates a index_mapped parameter
-        attr_name, index, key = self._split_attribute_index_mapping(item)
+        if not item.isalpha():
+            # Try matching the case where there are two indices
+            # this indicates a index_mapped parameter
+            attr_name, index, key = self._split_attribute_index_mapping(item)
 
-        # Check if this is an indexed_mapped attribute.
-        if (
-            key is not None
-            and index is not None
-            and attr_name in self._get_indexed_mapped_parameter_attributes()
-        ):
-            indexed_mapped_attr_value = getattr(self, attr_name)
-            try:
-                return indexed_mapped_attr_value[index][key]
-            except (IndexError, KeyError) as err:
-                raise MissingIndexedAttributeError(
-                    f"{err!s} '{item}' is out of bounds for indexed attribute '{attr_name}'"
-                )
+            # Check if this is an indexed_mapped attribute.
+            if (
+                key is not None
+                and index is not None
+                and attr_name in self._get_indexed_mapped_parameter_attributes()
+            ):
+                indexed_mapped_attr_value = getattr(self, attr_name)
+                try:
+                    return indexed_mapped_attr_value[index][key]
+                except (IndexError, KeyError) as err:
+                    raise MissingIndexedAttributeError(
+                        f"{err!s} '{item}' is out of bounds for indexed attribute '{attr_name}'"
+                    )
 
-        # Otherwise, try indexed attribute
-        # Separate the indexed attribute name from the list index.
-        attr_name, index = self._split_attribute_index(item)
+            # Otherwise, try indexed attribute
+            # Separate the indexed attribute name from the list index.
+            attr_name, index = self._split_attribute_index(item)
 
-        # Check if this is an indexed attribute.
-        if index is not None and attr_name in self._get_indexed_parameter_attributes():
-            indexed_attr_value = getattr(self, attr_name)
-            try:
-                return indexed_attr_value[index]
-            except IndexError:
-                raise MissingIndexedAttributeError(
-                    f"'{item}' is out of bounds for indexed attribute '{attr_name}'"
-                )
+            # Check if this is an indexed attribute.
+            if index is not None and attr_name in self._get_indexed_parameter_attributes():
+                indexed_attr_value = getattr(self, attr_name)
+                try:
+                    return indexed_attr_value[index]
+                except IndexError:
+                    raise MissingIndexedAttributeError(
+                        f"'{item}' is out of bounds for indexed attribute '{attr_name}'"
+                    )
 
         # Otherwise, forward the search to the next class in the MRO.
         try:
@@ -1355,33 +1356,33 @@ def filter(x):
         # sorts the attribute alphabetically by name. Here we want the order
         # to be the same as the declaration order, which is guaranteed by PEP 520,
         # starting from the parent class.
-        parameter_attributes = dict(
-            (name, descriptor)
+        return {
+            name: descriptor
             for c in reversed(inspect.getmro(cls))
             for name, descriptor in c.__dict__.items()
             if isinstance(descriptor, ParameterAttribute) and filter(descriptor)
-        )
-        return parameter_attributes
+        }
+
 
     @classmethod
     def _get_indexed_mapped_parameter_attributes(cls):
         """Shortcut to retrieve only IndexedMappedParameterAttributes."""
         return cls._get_parameter_attributes(
-            filter=lambda x: isinstance(x, IndexedMappedParameterAttribute)
+            filter=lambda x: type(x) is IndexedMappedParameterAttribute
         )
 
     @classmethod
     def _get_indexed_parameter_attributes(cls):
         """Shortcut to retrieve only IndexedParameterAttributes."""
         return cls._get_parameter_attributes(
-            filter=lambda x: isinstance(x, IndexedParameterAttribute)
+            filter=lambda x: type(x) is  IndexedParameterAttribute
         )
 
     @classmethod
     def _get_mapped_parameter_attributes(cls):
         """Shortcut to retrieve only IndexedParameterAttributes."""
         return cls._get_parameter_attributes(
-            filter=lambda x: isinstance(x, MappedParameterAttribute)
+            filter=lambda x: type(x) is MappedParameterAttribute
         )
 
     @classmethod
@@ -1588,14 +1589,14 @@ def __contains__(self, item):
         item
             SMIRKS of item in this ParameterList
         """
-        if isinstance(item, str):
+        if type(item) is str:
             # Special case for SMIRKS strings
             if item in [result.smirks for result in self]:
                 return True
         # Fall back to traditional access
         return list.__contains__(self, item)
 
-    def to_list(self, discard_cosmetic_attributes=True):
+    def to_list(self, discard_cosmetic_attributes: bool =True) -> list[dict]:
         """
         Render this ParameterList to a normal list, serializing each ParameterType object in it to dict.
 
@@ -1610,15 +1611,12 @@ def to_list(self, discard_cosmetic_attributes=True):
         parameter_list :list[dict]
             A serialized representation of a ParameterList, with each ParameterType it contains converted to dict.
         """
-        parameter_list = list()
-
-        for parameter in self:
-            parameter_dict = parameter.to_dict(
+        return [
+            parameter.to_dict(
                 discard_cosmetic_attributes=discard_cosmetic_attributes
             )
-            parameter_list.append(parameter_dict)
-
-        return parameter_list
+            for parameter in self
+        ]
 
 
 class VirtualSiteParameterList(ParameterList):
@@ -1905,6 +1903,31 @@ def __init__(
         # Initialize ParameterAttributes and cosmetic attributes.
         super().__init__(allow_cosmetic_attributes=allow_cosmetic_attributes, **kwargs)
 
+    def __hash__(self) -> int:
+        """
+        Hash a ParameterHandler and all of its contents (INCLUDING cosmetic attributes).
+
+        This method does not attempt to return the same hash for ParameterHandlers with equivalent
+        physics/chemistry but different cosmetic attributes or units. Instead this is a hash of all
+        of the ParameterHandler's contents, even if they don't affect system creation in any way.
+        """
+        import xmltodict
+
+        from openff.toolkit.utils.utils import convert_all_quantities_to_string
+
+        return hash(
+            xmltodict.unparse(
+                {
+                    'ROOT': convert_all_quantities_to_string(
+                        self.to_dict(
+                            discard_cosmetic_attributes=False,
+                        )
+                    )
+                }
+            )
+        )
+
+
     def _add_parameters(self, section_dict, allow_cosmetic_attributes=False):
         """
         Extend the ParameterList in this ParameterHandler using a SMIRNOFF data source.
@@ -1925,7 +1948,7 @@ def _add_parameters(self, section_dict, allow_cosmetic_attributes=False):
                 if key != element_name:
                     break
             # If there are multiple parameters, this will be a list. If there's just one, make it a list
-            if not (isinstance(val, list)):
+            if type(val) is not list:
                 val = [val]
 
             # If we're reading the parameter list, iterate through and attach units to
@@ -3854,13 +3877,12 @@ def _find_matches(
         assigned_matches_by_parent = self._find_matches_by_parent(entity)
         return_dict = {}
         for parent_index, assigned_parameters in assigned_matches_by_parent.items():
-            assigned_matches = []
-            for assigned_parameter, match_orientations in assigned_parameters:
-                for match in match_orientations:
-                    assigned_matches.append(
-                        ParameterHandler._Match(assigned_parameter, match)
-                    )
-            return_dict[(parent_index,)] = assigned_matches
+
+            return_dict[(parent_index,)] = [
+                ParameterHandler._Match(assigned_parameter, match)
+                for assigned_parameter, match_orientations in assigned_parameters
+                for match in match_orientations
+            ]
 
         return return_dict