search

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
315 stars 92 forks source link

`Molecule.add_hierarchy_scheme` allows registering hierarchy schemes with iterator names that conflict with existing attributes #1660

Closed mattwthompson closed 1 year ago

mattwthompson commented 1 year ago

In #1601 it was suggested that on iterators like Topology.residues:

If a HierarchyIterator tries to give itself a name like "atoms", which would conflict with Topology.atoms, then an error should be raised

However this behavior currently exists on Molecule. It could be caught when creating a Topology, but that wouldn't cover all cases.

In [1]: from openff.toolkit.topology.molecule import *

In [2]: molecule = Molecule.from_smiles("CCO")

In [3]: molecule.add_hierarchy_scheme(uniqueness_criteria=("magic",), iterator_name="atoms")
Out[3]: HierarchyScheme with uniqueness_criteria '('magic',)', iterator_name 'atoms', and 1 elements

In [4]: molecule.atoms
Out[4]:
[Atom(name=, atomic number=6),
 Atom(name=, atomic number=6),
 Atom(name=, atomic number=8),
 Atom(name=, atomic number=1),
 Atom(name=, atomic number=1),
 Atom(name=, atomic number=1),
 Atom(name=, atomic number=1),
 Atom(name=, atomic number=1),
 Atom(name=, atomic number=1)]

I don't think this should be allowed, and should probably be caught in add_hierarchy_scheme and not the __getattr__ magic.