openforcefield / openff-toolkit

The Open Forcefield Toolkit provides implementations of the SMIRNOFF format, parameterization engine, and other tools. Documentation available at http://open-forcefield-toolkit.readthedocs.io
http://openforcefield.org
MIT License
305 stars 90 forks source link

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

Closed mattwthompson closed 9 months ago

mattwthompson commented 9 months ago

https://github.com/openforcefield/openff-toolkit/issues/1186#issuecomment-1767128056

There are a few corner cases I've left uncovered - a full atom map, but one with ambiguous or confusing indices, or what to do if given a partial atom map - for simplicity. This just warns if the atom mapping is as long as the number of atoms.

Happy to update the language of the warning itself; I understand what I wrote but am not confident I'm using the best terms to describe the details.

I didn't think too deeply about adversarial cases, this handles the specific thing I care about:

In [1]: from openff.toolkit import Molecule

In [2]: Molecule.from_smiles("[H:2][O:1][H:3]")
<ipython-input-2-74b6c4f20a48>:1: AtomMappingWarning: 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`.
  Molecule.from_smiles("[H:2][O:1][H:3]")
Out[2]:
<IPython.core.display.SVG object>

In [3]: Molecule.from_mapped_smiles("[H:2][O:1][H:3]")
Out[3]: <IPython.core.display.SVG object>
codecov[bot] commented 9 months ago

Codecov Report

Merging #1747 (baadb96) into main (b4ec518) will decrease coverage by 0.03%. The diff coverage is 100.00%.

Additional details and impacted files
mattwthompson commented 9 months ago

Examples still failing and still my responsibility - not in context here, though

mattwthompson commented 9 months ago

Thank you!