Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add hierarchy scheme iterators to Topology #1661

Merged
merged 20 commits into from
Aug 22, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
c28d44f
Access hierarchy iterators on `Topology`
mattwthompson Jun 29, 2023
b263725
Add tests
mattwthompson Jun 29, 2023
3c55cf7
Update release history
mattwthompson Jun 29, 2023
500c098
Whoops, don't assume residues are the only scheme
mattwthompson Jun 29, 2023
d1ea622
Lint
mattwthompson Jun 29, 2023
986fbeb
Merge remote-tracking branch 'upstream/main' into add-topology-iterators
mattwthompson Jun 30, 2023
4b0769c
Band-aid to avoid `__deepcopy__` being looked up as a hierarchy scheme
mattwthompson Jun 30, 2023
671d208
Merge remote-tracking branch 'upstream/main' into add-topology-iterators
mattwthompson Jun 30, 2023
1619c93
Add RDKit decorator
mattwthompson Jul 3, 2023
36901ff
Merge remote-tracking branch 'upstream/main' into add-topology-iterators
mattwthompson Aug 14, 2023
52052de
Apply suggestions from code review
mattwthompson Aug 14, 2023
e80134e
Merge remote-tracking branch 'upstream/add-topology-iterators' into a…
mattwthompson Aug 14, 2023
977ffba
Update openff/toolkit/_tests/test_topology.py
mattwthompson Aug 14, 2023
a2a77c6
Merge remote-tracking branch 'upstream/add-topology-iterators' into a…
mattwthompson Aug 14, 2023
6109e74
Allow iterators with API clash
mattwthompson Aug 14, 2023
82640ba
Improve error message
mattwthompson Aug 14, 2023
aaf92f3
Catch `KeyError`
mattwthompson Aug 14, 2023
842ccdc
Do not install NAGL rc
mattwthompson Aug 14, 2023
ee2c13e
Merge branch 'no-nagl-rc' into add-topology-iterators
mattwthompson Aug 14, 2023
faab121
Merge remote-tracking branch 'upstream/main' into add-topology-iterators
mattwthompson Aug 14, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions docs/releasehistory.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ Releases follow the `major.minor.micro` scheme recommended by [PEP440](https://w
* `minor` increments add features but do not break API compatibility
* `micro` increments represent bugfix releases or improvements in documentation


## Current development

### API-breaking changes
Expand All @@ -17,8 +16,9 @@ Releases follow the `major.minor.micro` scheme recommended by [PEP440](https://w

### New features

### Improved documentation and warnings
- [PR #1662](https://github.com/openforcefield/openff-toolkit/pull/1662): Adds hierarchy scheme iterators to `Topology`, i.e. `Topology.residues`, when schemes of the same iterator name are defines on all constituent `Molecule`s.

### Improved documentation and warnings

## 0.14.3

Expand Down
101 changes: 101 additions & 0 deletions openff/toolkit/_tests/test_topology.py
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@
AtomNotInTopologyError,
BondNotInTopologyError,
DuplicateUniqueMoleculeError,
HierarchyIteratorNameConflictError,
IncompatibleUnitError,
InvalidBoxVectorsError,
InvalidPeriodicityError,
Expand Down Expand Up @@ -1781,6 +1782,106 @@ def test_add_with_constraints(self):
)


def residue_equality_check(residue1, residue2):
"""Hack to work around #1659."""
assert residue1.n_atoms == residue2.n_atoms
assert residue1.residue_number == residue2.residue_number
assert residue1.residue_name == residue2.residue_name
assert residue1.chain_id == residue2.chain_id
assert residue1.insertion_code == residue2.insertion_code


class TestTopologyHierarchyIterators:
@requires_rdkit
def test_single_small_peptide(
self,
):
protein_path = get_data_file_path("proteins/ace-ala-nh2.pdb")

topology = Topology.from_pdb(protein_path)
molecule = Molecule.from_polymer_pdb(protein_path)

assert hasattr(topology, "residues")

assert len(topology.residues) == len(molecule.residues)

for topology_residue, molecule_residue in zip(
topology.residues,
molecule.residues,
):
residue_equality_check(
topology_residue,
molecule_residue,
)

def test_no_iterators(self):
topology = Molecule.from_smiles("CCO").to_topology()

with pytest.raises(
AttributeError,
match="'Topology' object has no attribute 'residues'",
):
topology.residues

@requires_rdkit
def test_error_when_mixed_iterators(
self,
):
"""Test that an error is raised when a topology has molecules with and without an iterator."""
protein_path = get_data_file_path("proteins/ace-ala-nh2.pdb")

topology = Topology.from_pdb(protein_path)
topology.add_molecule(Molecule.from_smiles("CCO"))

with pytest.raises(
AttributeError,
match="'Topology' object has no attribute 'residues'",
):
topology.residues

def test_molecule_order_wins_over_residue_order(
self,
):
"""Test that no effort is made to sort residues by residue number across molecules."""
protein_path = get_data_file_path("proteins/ace-ala-nh2.pdb")

topology = Topology.from_molecules(
[
Molecule.from_polymer_pdb(protein_path),
Molecule.from_polymer_pdb(protein_path),
]
)

assert (
topology.residues[0].residue_number == topology.residues[3].residue_number
)
assert (
topology.residues[1].residue_number == topology.residues[4].residue_number
)
assert (
topology.residues[2].residue_number == topology.residues[5].residue_number
)

assert int(topology.residues[2].residue_number) > int(
topology.residues[3].residue_number
)

@pytest.mark.skip(reason="Undefined behavior upstream, see #1660")
def test_clashing_hierarchy_scheme_iterator_name(
self,
):
molecule = Molecule.from_smiles("CCO")
molecule.add_hierarchy_scheme(
uniqueness_criteria=("magic",),
iterator_name="atoms",
)

topology = molecule.to_topology()

with pytest.raises(HierarchyIteratorNameConflictError):
topology.atoms


class TestTopologySerialization:
@pytest.fixture
def oleic_acid(self):
Expand Down
19 changes: 19 additions & 0 deletions openff/toolkit/topology/topology.py
Original file line number Diff line number Diff line change
Expand Up @@ -2427,3 +2427,22 @@ def hierarchy_iterator(
if hasattr(molecule, iter_name):
for item in getattr(molecule, iter_name):
yield item

def __getattr__(self, name: str) -> List["HierarchyElement"]:
"""If a requested attribute is not found, check the hierarchy schemes"""
# Avoid attempting to process dunder methods as hierarchy scheme iterator names
if name.startswith("__"):
raise AttributeError

mattwthompson marked this conversation as resolved.
Show resolved Hide resolved
try:
return [
element
for molecule in self.molecules
for element in molecule.hierarchy_schemes[name].hierarchy_elements
]
except (KeyError, AttributeError) as error:
raise AttributeError(
f"'{self.__class__.__name__}' object has no attribute '{name}'. If looking for a "
"`HierarchyScheme` iterator, not all molecules in this topology have an interator "
f"name {name} defined."
) from error