Warn if Molecule.from_smiles is provided a full atom map (#1747)

* Warn if `Molecule.from_smiles` is provided a full atom map

* Test behaves differently with RDKit, so avoid it

* Update release history

docs/releasehistory.md

@@ -20,9 +20,10 @@ Releases follow the `major.minor.micro` scheme recommended by [PEP440](https://w
 
 ## Current development
 
+- [PR #1747](https://github.com/openforcefield/openff-toolkit/pull/1747): Warns if a SMILES with full atom mappings is passed to `Moleucle.from_smiles`, which does not use the atom map for atom ordering (`Molecule.from_mapped_smiles` does).
 - [PR #1731](https://github.com/openforcefield/openff-toolkit/pull/1731): Suppot SMIRNOFF vdW version 0.5.
 
-## 0.14.4 
+## 0.14.4
 
 ### Behavior changes
 

openff/toolkit/_tests/test_molecule.py

@@ -17,6 +17,7 @@
 import pathlib
 import pickle
 import re
+import warnings
 from tempfile import NamedTemporaryFile
 from unittest.mock import Mock
 
@@ -56,6 +57,7 @@
 )
 from openff.toolkit.utils import get_data_file_path
 from openff.toolkit.utils.exceptions import (
+    AtomMappingWarning,
     HierarchyIteratorNameConflictError,
     IncompatibleShapeError,
     IncompatibleTypeError,
@@ -2875,6 +2877,24 @@ def test_from_mapped_smiles_partial(self, toolkit_class):
         ):
             Molecule.from_mapped_smiles("[Cl:1][Cl]", toolkit_registry=toolkit_class())
 
+    def test_smiles_with_full_map_warning(
+        self,
+    ):
+        with pytest.warns(AtomMappingWarning):
+            Molecule.from_smiles("[H:2][O:1][H:3]")
+
+    @requires_openeye
+    def test_smiles_with_partial_map_no_warning(
+        self,
+    ):
+        """Ensure the 'do you mean from_mapped_smiles?' warning is not emitted with a partial map"""
+        with warnings.catch_warnings():
+            # This turns warnings into exceptions - one way of implementing
+            # "ensure there is not a warning raised"
+            warnings.simplefilter("error")
+
+            Molecule.from_smiles("H[O:1]H")
+
     @pytest.mark.parametrize("molecule", mini_drug_bank())
     def test_n_atoms(self, molecule):
         """Test n_atoms property"""

openff/toolkit/topology/molecule.py

@@ -58,6 +58,7 @@
 
 from openff.toolkit.utils.constants import DEFAULT_AROMATICITY_MODEL
 from openff.toolkit.utils.exceptions import (
+    AtomMappingWarning,
     BondExistsError,
     HierarchyIteratorNameConflictError,
     HierarchySchemeNotFoundException,
@@ -1880,6 +1881,17 @@ def from_smiles(
                 f"Got {type(toolkit_registry)}"
             )
 
+        if "atom_map" in molecule._properties:
+            if len(molecule._properties["atom_map"]) == molecule.n_atoms:
+                warnings.warn(
+                    "Warning! Fully mapped SMILES pattern passed to `from_smiles`. The atom map is "
+                    "stored as a property in `Molecule._properties`, but these indices are NOT "
+                    "used to determine atom ordering. To use these indices for atom ordering, use "
+                    "`Molecule.from_mapped_smiles`.",
+                    AtomMappingWarning,
+                    stacklevel=2,
+                )
+
         return molecule
 
     def _is_exactly_the_same_as(self, other):
@@ -4534,13 +4546,19 @@ def from_mapped_smiles(
 
         # create the molecule from the smiles and check we have the right number of indexes
         # in the mapped SMILES
+        warnings.filterwarnings("ignore", category=AtomMappingWarning)
+
         offmol = cls.from_smiles(
             mapped_smiles,
             hydrogens_are_explicit=True,
             toolkit_registry=toolkit_registry,
             allow_undefined_stereo=allow_undefined_stereo,
         )
 
+        # https://stackoverflow.com/a/53763710
+        # this might be better: https://docs.python.org/3/library/warnings.html#warnings.catch_warnings
+        warnings.filterwarnings("default", category=AtomMappingWarning)
+
         # check we found some mapping and remove it as we do not want to expose atom maps
         try:
             mapping = offmol._properties.pop("atom_map")

openff/toolkit/utils/exceptions.py

@@ -101,6 +101,11 @@ class SMILESParseError(OpenFFToolkitException, ValueError):
     """The record could not be parsed into the given format"""
 
 
+# TODO: Should warnings inherit from a sort of OpenFFToolkitWarning?
+class AtomMappingWarning(UserWarning):
+    """A warning when dealing with atom maping or indices."""
+
+
 class InChIParseError(MoleculeParseError, RuntimeError):
     """The InChI record could not be parsed."""