Merge pull request #786 from openforcefield/benchmarking-fixes

Reliability fixes for benchmarking work and 0.8.2 releasenotes

docs/releasehistory.rst

@@ -7,15 +7,25 @@ Releases follow the ``major.minor.micro`` scheme recommended by `PEP440 <https:/
 * ``minor`` increments add features but do not break API compatibility
 * ``micro`` increments represent bugfix releases or improvements in documentation
 
-0.8.2 - Current development
----------------------------
+0.8.2 - Bugfix release
+----------------------
 
 Bugfixes
 """"""""
-- `PR #789 <https://github.com/openforcefield/openforcefield/pull/xyz>`_: Fixes bug
-  when creating
+- `PR #786 <https://github.com/openforcefield/openforcefield/pull/xyz>`_: Fixes `Issue #785
+  <https://github.com/openforcefield/openforcefield/issues/785>`_ where RDKitToolkitWrapper would
+  sometimes expect stereochemistry to be defined for non-stereogenic bonds when loading from
+  SDF.
+- `PR #786 <https://github.com/openforcefield/openforcefield/pull/786>`_: Fixes an issue where
+  using the :py:class:`Molecule <openforcefield.topology.Molecule>` copy constructor
+  (``newmol = Molecule(oldmol)``) would result
+  in the copy sharing the same ``.properties`` dict as the original (as in, changes to the
+  ``.properties`` dict of the copy would be reflected in the original).
+- `PR #789 <https://github.com/openforcefield/openforcefield/pull/789>`_: Fixes a regression noted in
+  `Issue #788 <https://github.com/openforcefield/openforcefield/issues/788>`_
+  where creating
   :py:class:`vdWHandler.vdWType <openforcefield.typing.engines.smirnoff.parameters.vdWHandler.vdWType>`
-  from scratch using dicts of strings.
+  or setting ``sigma`` or ``rmin_half`` using Quantities represented as strings resulted in an error.
 
 
 0.8.1 - Bugfix and minor feature release
@@ -366,7 +376,7 @@ Bugfixes
 - `PR #631 <https://github.com/openforcefield/openforcefield/pull/631>`_: Fixes a bug in which calling
   :py:class:`unit_to_string <openforcefield.utils.utils.unit_to_string>` returned
   ``None`` when the unit is dimensionless. Now ``"dimensionless"`` is returned.
-- `PR #630 <https://github.com/openforcefield/openforcefield/pull/630>`_: Closes issue `Issue #629 
+- `PR #630 <https://github.com/openforcefield/openforcefield/pull/630>`_: Closes issue `Issue #629
   <https://github.com/openforcefield/openforcefield/issues/629>`_ in which the wrong exception is raised when
   attempting to instantiate a :py:class:`ForceField <openforcefield.typing.engines.smirnoff.forcefield.ForceField>`
   from an unparsable string.

openforcefield/data/molecules/ethene_rdkit.sdf

@@ -0,0 +1,17 @@
+
+     RDKit          3D
+
+  6  5  0  0  0  0  0  0  0  0999 V2000
+    0.6518    0.0477    0.0697 C   0  0  0  0  0  0  0  0  0  0  0  0
+   -0.6641   -0.0571   -0.0745 C   0  0  0  0  0  0  0  0  0  0  0  0
+    1.0897    0.9530    0.4769 H   0  0  0  0  0  0  0  0  0  0  0  0
+    1.3429   -0.7290   -0.1978 H   0  0  0  0  0  0  0  0  0  0  0  0
+   -1.3260    0.7492    0.2074 H   0  0  0  0  0  0  0  0  0  0  0  0
+   -1.0943   -0.9637   -0.4817 H   0  0  0  0  0  0  0  0  0  0  0  0
+  1  2  2  3
+  1  3  1  0
+  1  4  1  0
+  2  5  1  0
+  2  6  1  0
+M  END
+$$$$

openforcefield/tests/test_molecule.py

@@ -456,6 +456,11 @@ def test_create_copy(self, molecule):
         molecule_copy = Molecule(molecule)
         assert molecule_copy == molecule
 
+        # Test that the "properties" dict of both molecules is unique
+        # (see https://github.com/openforcefield/openforcefield/pull/786)
+        molecule_copy.properties["aaa"] = "bbb"
+        assert "aaa" not in molecule.properties
+
     @pytest.mark.parametrize("toolkit", [OpenEyeToolkitWrapper, RDKitToolkitWrapper])
     @pytest.mark.parametrize("molecule", mini_drug_bank())
     def test_to_from_smiles(self, molecule, toolkit):

openforcefield/tests/test_toolkits.py

@@ -2025,6 +2025,15 @@ def test_write_sdf_no_charges(self):
         # out "n/a" (or another placeholder) in the partial charge block atoms without charges.
         assert ">  <atom.dprop.PartialCharge>" not in sdf_text
 
+    def test_read_ethene_sdf(self):
+        """
+        Test that RDKitToolkitWrapper can load an ethene molecule without complaining about bond stereo.
+        See https://github.com/openforcefield/openforcefield/issues/785
+        """
+        ethene_file_path = get_data_file_path("molecules/ethene_rdkit.sdf")
+        toolkit_wrapper = RDKitToolkitWrapper()
+        toolkit_wrapper.from_file(ethene_file_path, file_format="sdf")
+
     def test_load_multiconformer_sdf_as_separate_molecules(self):
         """
         Test RDKitToolkitWrapper for reading a "multiconformer" SDF, which the OFF

openforcefield/tests/utils.py

@@ -1382,6 +1382,10 @@ def compare_system_parameters(
                 continue
             force_pairs[force.__class__.__name__].append(force)
 
+    # TODO: ParmEd-derived system representations separate out vdW from ES (putting ES in NonbondedForce
+    # and vdW in CustomNonbondedForce with precomputed mixing parameters). We'll need to update this to
+    # somehow account for the difference.
+
     # Compare all pairs of forces
     for force_name, (force1, force2) in force_pairs.items():
         # Select the parameters to ignore.

openforcefield/topology/molecule.py

@@ -2193,7 +2193,10 @@ def _copy_initializer(self, other):
 
         """
         # assert isinstance(other, type(self)), "can only copy instances of {}".format(type(self))
-        other_dict = other.to_dict()
+
+        # Run a deepcopy here so that items that were _always_ dict (like other.properties) will
+        # not have any references to the old molecule
+        other_dict = deepcopy(other.to_dict())
         self._initialize_from_dict(other_dict)
 
     def __eq__(self, other):

openforcefield/utils/toolkits.py

@@ -3541,6 +3541,8 @@ def from_rdkit(self, rdmol, allow_undefined_stereo=False, _cls=None):
             offb_idx = map_bonds[rdb_idx]
             offb = offmol.bonds[offb_idx]
             # determine if stereochemistry is needed
+            # Note that RDKit has 6 possible values of bond stereo: CIS, TRANS, E, Z, ANY, or NONE
+            # The logic below assumes that "ANY" and "NONE" mean the same thing.
             stereochemistry = None
             tag = rdb.GetStereo()
             if tag == Chem.BondStereo.STEREOZ:
@@ -4108,15 +4110,29 @@ def _find_undefined_stereo_bonds(rdmol):
         # assign Bond.STEREOANY to unspecific bond, which make subsequent calls
         # of Chem.AssignStereochemistry ignore the bond even if there are
         # ENDDOWNRIGHT/ENDUPRIGHT bond direction indications.
-        rdmol = copy.deepcopy(rdmol)
+        rdmol_copy = copy.deepcopy(rdmol)
+
+        # Clear any previous assignments on the bonds, since FindPotentialStereo may not overwrite it
+        for bond in rdmol_copy.GetBonds():
+            bond.SetStereo(Chem.BondStereo.STEREONONE)
 
         # This function assigns Bond.GetStereo() == Bond.STEREOANY to bonds with
-        # undefined stereochemistry.
-        Chem.FindPotentialStereoBonds(rdmol)
+        # possible stereochemistry.
+        Chem.FindPotentialStereoBonds(rdmol_copy, cleanIt=True)
 
+        # Any TRULY stereogenic bonds in the molecule are now marked as STEREOANY in rdmol_copy.
+        # Iterate through all the bonds, and for the ones where rdmol_copy is marked as STEREOANY,
+        # ensure that they are cis/trans/E/Z (tested here be ensuring that they're NOT either
+        # # of the other possible types (NONE or ANY))
         undefined_bond_indices = []
-        for bond_idx, bond in enumerate(rdmol.GetBonds()):
-            if bond.GetStereo() == Chem.BondStereo.STEREOANY:
+        for bond_idx, (orig_bond, repercieved_bond) in enumerate(
+            zip(rdmol.GetBonds(), rdmol_copy.GetBonds())
+        ):
+            # print(repercieved_bond.GetStereo(), orig_bond.GetStereo())
+            if (repercieved_bond.GetStereo() == Chem.BondStereo.STEREOANY) and (
+                (orig_bond.GetStereo() == Chem.BondStereo.STEREOANY)
+                or (orig_bond.GetStereo() == Chem.BondStereo.STEREONONE)
+            ):
                 undefined_bond_indices.append(bond_idx)
         return undefined_bond_indices