From 6ee7c17f1d41430e7bbc2ef48518549e78eb76cb Mon Sep 17 00:00:00 2001 From: Lily Wang Date: Fri, 25 Oct 2024 16:31:23 +1100 Subject: [PATCH 01/30] add tests --- .../MDAnalysisTests/guesser/test_base.py | 52 ++++++++++++++++++- 1 file changed, 51 insertions(+), 1 deletion(-) diff --git a/testsuite/MDAnalysisTests/guesser/test_base.py b/testsuite/MDAnalysisTests/guesser/test_base.py index b429826647..af10f9f6ad 100644 --- a/testsuite/MDAnalysisTests/guesser/test_base.py +++ b/testsuite/MDAnalysisTests/guesser/test_base.py @@ -27,10 +27,11 @@ from MDAnalysis.core.topology import Topology from MDAnalysis.core.topologyattrs import Masses, Atomnames, Atomtypes import MDAnalysis.tests.datafiles as datafiles +from MDAnalysis.exceptions import NoDataError from numpy.testing import assert_allclose, assert_equal -class TesttBaseGuesser(): +class TestBaseGuesser(): def test_get_guesser(self): class TestGuesser1(GuesserBase): @@ -100,3 +101,52 @@ def test_partial_guess_attr_with_unknown_no_value_label(self): top = Topology(4, 1, 1, attrs=[names, types, ]) u = mda.Universe(top, to_guess=['types']) assert_equal(u.atoms.types, ['', '', '', '']) + + def test_guess_topology_objects_existing(self): + u = mda.Universe(datafiles.CONECT, to_guess=["bonds"]) + assert len(u.atoms.bonds) == 1922 + assert list(u.bonds[0].indices) == [0, 1] + + # delete some bonds + u.delete_bonds(u.atoms.bonds[:100]) + assert len(u.atoms.bonds) == 1822 + assert list(u.bonds[0].indices) == [94, 99] + + all_indices = [tuple(x.indices) for x in u.bonds] + assert (0, 1) not in all_indices + + # guess old bonds back + u.guess_TopologyAttrs("default", to_guess=["bonds"]) + assert len(u.atoms.bonds) == 1922 + # check TopologyGroup contains new (old) bonds + assert list(u.bonds[0].indices) == [0, 1] + + def test_guess_topology_objects_force(self): + u = mda.Universe(datafiles.CONECT, force_guess=["bonds"]) + assert len(u.atoms.bonds) == 1922 + + with pytest.raises(NoDataError): + u.atoms.angles + + + def test_guess_topology_objects_out_of_order_init(self): + u = mda.Universe( + datafiles.PDB_small, + to_guess=["dihedrals", "angles", "bonds"], + guess_bonds=False + ) + assert len(u.atoms.angles) == 6123 + assert len(u.atoms.dihedrals) == 8921 + + def test_guess_topology_objects_out_of_order_guess(self): + u = mda.Universe(datafiles.PDB_small) + with pytest.raises(NoDataError): + u.atoms.angles + + u.guess_TopologyAttrs( + "default", + to_guess=["dihedrals", "angles", "bonds"] + ) + assert len(u.atoms.angles) == 6123 + assert len(u.atoms.dihedrals) == 8921 + From ada696759d07f3e603d67eef9c3a545d47350500 Mon Sep 17 00:00:00 2001 From: Lily Wang Date: Fri, 25 Oct 2024 16:33:22 +1100 Subject: [PATCH 02/30] special case connections --- package/MDAnalysis/guesser/base.py | 43 ++++++++++++++++++------------ 1 file changed, 26 insertions(+), 17 deletions(-) diff --git a/package/MDAnalysis/guesser/base.py b/package/MDAnalysis/guesser/base.py index aab0723aaa..f307a661b3 100644 --- a/package/MDAnalysis/guesser/base.py +++ b/package/MDAnalysis/guesser/base.py @@ -36,9 +36,9 @@ .. autofunction:: get_guesser """ -from .. import _GUESSERS +from .. import _GUESSERS, _TOPOLOGY_ATTRS +from ..core.topologyattrs import _Connection import numpy as np -from .. import _TOPOLOGY_ATTRS import logging from typing import Dict import copy @@ -136,6 +136,11 @@ def guess_attr(self, attr_to_guess, force_guess=False): NDArray of guessed values """ + try: + guesser_method = self._guesser_methods[attr_to_guess] + except KeyError: + raise ValueError(f'{type(self).__name__} cannot guess the following ' + f'attribute: {attr_to_guess}') # check if the topology already has the attribute to partially guess it if hasattr(self._universe.atoms, attr_to_guess) and not force_guess: @@ -143,23 +148,27 @@ def guess_attr(self, attr_to_guess, force_guess=False): getattr(self._universe.atoms, attr_to_guess, None)) top_attr = _TOPOLOGY_ATTRS[attr_to_guess] - - empty_values = top_attr.are_values_missing(attr_values) - - if True in empty_values: - # pass to the guesser_method boolean mask to only guess the - # empty values - attr_values[empty_values] = self._guesser_methods[attr_to_guess]( - indices_to_guess=empty_values) - return attr_values - + if issubclass(top_attr, _Connection): + return guesser_method() + else: - logger.info( - f'There is no empty {attr_to_guess} values. Guesser did ' - f'not guess any new values for {attr_to_guess} attribute') - return None + empty_values = top_attr.are_values_missing(attr_values) + + if True in empty_values: + # pass to the guesser_method boolean mask to only guess the + # empty values + attr_values[empty_values] = guesser_method( + indices_to_guess=empty_values + ) + return attr_values + + else: + logger.info( + f'There is no empty {attr_to_guess} values. Guesser did ' + f'not guess any new values for {attr_to_guess} attribute') + return None else: - return np.array(self._guesser_methods[attr_to_guess]()) + return np.array(guesser_method()) def get_guesser(context, u=None, **kwargs): From 5ce43e035215d9c22dd5da62ecad7d90950c25fe Mon Sep 17 00:00:00 2001 From: Lily Wang Date: Fri, 25 Oct 2024 16:34:00 +1100 Subject: [PATCH 03/30] pep8 tests --- testsuite/MDAnalysisTests/guesser/test_base.py | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/testsuite/MDAnalysisTests/guesser/test_base.py b/testsuite/MDAnalysisTests/guesser/test_base.py index af10f9f6ad..d5e093739b 100644 --- a/testsuite/MDAnalysisTests/guesser/test_base.py +++ b/testsuite/MDAnalysisTests/guesser/test_base.py @@ -114,7 +114,7 @@ def test_guess_topology_objects_existing(self): all_indices = [tuple(x.indices) for x in u.bonds] assert (0, 1) not in all_indices - + # guess old bonds back u.guess_TopologyAttrs("default", to_guess=["bonds"]) assert len(u.atoms.bonds) == 1922 @@ -128,7 +128,6 @@ def test_guess_topology_objects_force(self): with pytest.raises(NoDataError): u.atoms.angles - def test_guess_topology_objects_out_of_order_init(self): u = mda.Universe( datafiles.PDB_small, @@ -137,7 +136,7 @@ def test_guess_topology_objects_out_of_order_init(self): ) assert len(u.atoms.angles) == 6123 assert len(u.atoms.dihedrals) == 8921 - + def test_guess_topology_objects_out_of_order_guess(self): u = mda.Universe(datafiles.PDB_small) with pytest.raises(NoDataError): @@ -149,4 +148,3 @@ def test_guess_topology_objects_out_of_order_guess(self): ) assert len(u.atoms.angles) == 6123 assert len(u.atoms.dihedrals) == 8921 - From 7db0f6aae1c694c9efbe427748ba56d945ddd755 Mon Sep 17 00:00:00 2001 From: Lily Wang Date: Fri, 25 Oct 2024 16:40:16 +1100 Subject: [PATCH 04/30] add failing force_guess bond test --- testsuite/MDAnalysisTests/guesser/test_base.py | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/testsuite/MDAnalysisTests/guesser/test_base.py b/testsuite/MDAnalysisTests/guesser/test_base.py index d5e093739b..8087302e61 100644 --- a/testsuite/MDAnalysisTests/guesser/test_base.py +++ b/testsuite/MDAnalysisTests/guesser/test_base.py @@ -148,3 +148,16 @@ def test_guess_topology_objects_out_of_order_guess(self): ) assert len(u.atoms.angles) == 6123 assert len(u.atoms.dihedrals) == 8921 + + def test_force_guess_overwrites_existing_bonds(self): + u = mda.Universe(datafiles.PDB_conect) + assert len(u.atoms.bonds) == 8 + + # PDB_conect only has Cs. This low radius should find no bonds + cvdw = {"C": 0.1} + u.guess_TopologyAttrs("default", to_guess=["bonds"], vdwradii=cvdw) + assert len(u.atoms.bonds) == 8 + + # Now force guess bonds + u.guess_TopologyAttrs("default", force_guess=["bonds"], vdwradii=cvdw) + assert len(u.atoms.bonds) == 0 From f22a9a6b799e87c0acfaa160ca5d2f0869373597 Mon Sep 17 00:00:00 2001 From: Lily Wang Date: Fri, 25 Oct 2024 18:10:27 +1100 Subject: [PATCH 05/30] add force guessing removal --- package/MDAnalysis/core/topologyattrs.py | 1 - package/MDAnalysis/core/universe.py | 57 ++++++++++++------- package/MDAnalysis/guesser/base.py | 49 +++++++++------- .../MDAnalysisTests/guesser/test_base.py | 17 +++--- 4 files changed, 78 insertions(+), 46 deletions(-) diff --git a/package/MDAnalysis/core/topologyattrs.py b/package/MDAnalysis/core/topologyattrs.py index 5e2621dc63..1cbc83c2ed 100644 --- a/package/MDAnalysis/core/topologyattrs.py +++ b/package/MDAnalysis/core/topologyattrs.py @@ -3117,7 +3117,6 @@ def _add_bonds(self, values, types=None, guessed=True, order=None): guessed = itertools.cycle((guessed,)) if order is None: order = itertools.cycle((None,)) - existing = set(self.values) for v, t, g, o in zip(values, types, guessed, order): if v not in existing: diff --git a/package/MDAnalysis/core/universe.py b/package/MDAnalysis/core/universe.py index c0ac6bcf6f..1ffa426256 100644 --- a/package/MDAnalysis/core/universe.py +++ b/package/MDAnalysis/core/universe.py @@ -84,7 +84,10 @@ Atom, Residue, Segment, AtomGroup, ResidueGroup, SegmentGroup) from .topology import Topology -from .topologyattrs import AtomAttr, ResidueAttr, SegmentAttr, BFACTOR_WARNING +from .topologyattrs import ( + AtomAttr, ResidueAttr, SegmentAttr, + BFACTOR_WARNING, _Connection +) from .topologyobjects import TopologyObject from ..guesser.base import get_guesser @@ -1132,7 +1135,6 @@ def _add_topology_objects(self, object_type, values, types=None, guessed=False, self.add_TopologyAttr(object_type, []) attr = getattr(self._topology, object_type) - attr._add_bonds(indices, types=types, guessed=guessed, order=order) def add_bonds(self, values, types=None, guessed=False, order=None): @@ -1183,6 +1185,14 @@ def add_bonds(self, values, types=None, guessed=False, order=None): """ self._add_topology_objects('bonds', values, types=types, guessed=guessed, order=order) + self._invalidate_bond_related_caches() + + def _invalidate_bond_related_caches(self): + """ + Invalidate caches related to bonds and fragments. + + .. versionadded: 2.8.0 + """ # Invalidate bond-related caches self._cache.pop('fragments', None) self._cache['_valid'].pop('fragments', None) @@ -1259,7 +1269,7 @@ def _delete_topology_objects(self, object_type, values): Parameters ---------- object_type : {'bonds', 'angles', 'dihedrals', 'impropers'} - The type of TopologyObject to add. + The type of TopologyObject to delete. values : iterable of tuples, AtomGroups, or TopologyObjects; or TopologyGroup An iterable of: tuples of atom indices, or AtomGroups, or TopologyObjects. @@ -1282,7 +1292,6 @@ def _delete_topology_objects(self, object_type, values): attr = getattr(self._topology, object_type) except AttributeError: raise ValueError('There are no {} to delete'.format(object_type)) - attr._delete_bonds(indices) def delete_bonds(self, values): @@ -1323,10 +1332,7 @@ def delete_bonds(self, values): .. versionadded:: 1.0.0 """ self._delete_topology_objects('bonds', values) - # Invalidate bond-related caches - self._cache.pop('fragments', None) - self._cache['_valid'].pop('fragments', None) - self._cache['_valid'].pop('fragindices', None) + self._invalidate_bond_related_caches() def delete_angles(self, values): """Delete Angles from this Universe. @@ -1552,7 +1558,10 @@ def guess_TopologyAttrs( # in the same order that the user provided total_guess = list(dict.fromkeys(total_guess)) - objects = ['bonds', 'angles', 'dihedrals', 'impropers'] + objects = set( + topattr.attrname for topattr in _TOPOLOGY_ATTRS.values() + if issubclass(topattr, _Connection) + ) # Checking if the universe is empty to avoid errors # from guesser methods @@ -1579,16 +1588,26 @@ def guess_TopologyAttrs( fg = attr in force_guess values = guesser.guess_attr(attr, fg) - if values is not None: - if attr in objects: - self._add_topology_objects( - attr, values, guessed=True) - else: - guessed_attr = _TOPOLOGY_ATTRS[attr](values, True) - self.add_TopologyAttr(guessed_attr) - logger.info( - f'attribute {attr} has been guessed' - ' successfully.') + # None indicates no additional guessing was done + if values is None: + continue + if attr in objects: + # delete existing connections if they exist + if fg and hasattr(self.atoms, attr): + group = getattr(self.atoms, attr) + self.delete_bonds(group) + self._delete_topology_objects(attr, group) + # this method appends to existing bonds + self._add_topology_objects( + attr, values, guessed=True) + if attr == "bonds": + self._invalidate_bond_related_caches() + else: + guessed_attr = _TOPOLOGY_ATTRS[attr](values, True) + self.add_TopologyAttr(guessed_attr) + logger.info( + f'attribute {attr} has been guessed' + ' successfully.') else: raise ValueError(f'{context} guesser can not guess the' diff --git a/package/MDAnalysis/guesser/base.py b/package/MDAnalysis/guesser/base.py index f307a661b3..2bd9cb8423 100644 --- a/package/MDAnalysis/guesser/base.py +++ b/package/MDAnalysis/guesser/base.py @@ -136,37 +136,48 @@ def guess_attr(self, attr_to_guess, force_guess=False): NDArray of guessed values """ + try: + top_attr = _TOPOLOGY_ATTRS[attr_to_guess] + except KeyError: + raise KeyError( + f"{attr_to_guess} is not a recognized MDAnalysis " + "topology attribute" + ) try: guesser_method = self._guesser_methods[attr_to_guess] except KeyError: raise ValueError(f'{type(self).__name__} cannot guess the following ' f'attribute: {attr_to_guess}') + + # make attribute to guess plural + attr_to_guess = top_attr.attrname + + # Connection attributes should be just returned as they are always + # appended to the Universe. ``force_guess`` handling should happen + # at Universe level. + if issubclass(top_attr, _Connection): + return guesser_method() # check if the topology already has the attribute to partially guess it if hasattr(self._universe.atoms, attr_to_guess) and not force_guess: attr_values = np.array( getattr(self._universe.atoms, attr_to_guess, None)) - top_attr = _TOPOLOGY_ATTRS[attr_to_guess] - if issubclass(top_attr, _Connection): - return guesser_method() - + empty_values = top_attr.are_values_missing(attr_values) + + if True in empty_values: + # pass to the guesser_method boolean mask to only guess the + # empty values + attr_values[empty_values] = guesser_method( + indices_to_guess=empty_values + ) + return attr_values + else: - empty_values = top_attr.are_values_missing(attr_values) - - if True in empty_values: - # pass to the guesser_method boolean mask to only guess the - # empty values - attr_values[empty_values] = guesser_method( - indices_to_guess=empty_values - ) - return attr_values - - else: - logger.info( - f'There is no empty {attr_to_guess} values. Guesser did ' - f'not guess any new values for {attr_to_guess} attribute') - return None + logger.info( + f'There is no empty {attr_to_guess} values. Guesser did ' + f'not guess any new values for {attr_to_guess} attribute') + return None else: return np.array(guesser_method()) diff --git a/testsuite/MDAnalysisTests/guesser/test_base.py b/testsuite/MDAnalysisTests/guesser/test_base.py index 8087302e61..3e3c9bef51 100644 --- a/testsuite/MDAnalysisTests/guesser/test_base.py +++ b/testsuite/MDAnalysisTests/guesser/test_base.py @@ -30,6 +30,8 @@ from MDAnalysis.exceptions import NoDataError from numpy.testing import assert_allclose, assert_equal +from MDAnalysis import _TOPOLOGY_ATTRS, _GUESSERS + class TestBaseGuesser(): @@ -150,14 +152,15 @@ def test_guess_topology_objects_out_of_order_guess(self): assert len(u.atoms.dihedrals) == 8921 def test_force_guess_overwrites_existing_bonds(self): - u = mda.Universe(datafiles.PDB_conect) - assert len(u.atoms.bonds) == 8 + u = mda.Universe(datafiles.CONECT) + assert len(u.atoms.bonds) == 72 - # PDB_conect only has Cs. This low radius should find no bonds - cvdw = {"C": 0.1} - u.guess_TopologyAttrs("default", to_guess=["bonds"], vdwradii=cvdw) - assert len(u.atoms.bonds) == 8 + # This low radius should find no bonds + vdw = dict.fromkeys(set(u.atoms.types), 0.1) + u.guess_TopologyAttrs("default", to_guess=["bonds"], vdwradii=vdw) + assert len(u.atoms.bonds) == 72 # Now force guess bonds - u.guess_TopologyAttrs("default", force_guess=["bonds"], vdwradii=cvdw) + u.guess_TopologyAttrs("default", force_guess=["bonds"], vdwradii=vdw) assert len(u.atoms.bonds) == 0 + assert False From e21059c67c2c3e70d01e58d03454c64c17fd9b61 Mon Sep 17 00:00:00 2001 From: Lily Wang Date: Fri, 25 Oct 2024 18:26:50 +1100 Subject: [PATCH 06/30] fix pep8 --- package/MDAnalysis/core/universe.py | 2 +- package/MDAnalysis/guesser/base.py | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/package/MDAnalysis/core/universe.py b/package/MDAnalysis/core/universe.py index 1ffa426256..ed2c34354d 100644 --- a/package/MDAnalysis/core/universe.py +++ b/package/MDAnalysis/core/universe.py @@ -1186,7 +1186,7 @@ def add_bonds(self, values, types=None, guessed=False, order=None): self._add_topology_objects('bonds', values, types=types, guessed=guessed, order=order) self._invalidate_bond_related_caches() - + def _invalidate_bond_related_caches(self): """ Invalidate caches related to bonds and fragments. diff --git a/package/MDAnalysis/guesser/base.py b/package/MDAnalysis/guesser/base.py index 2bd9cb8423..7360df7b43 100644 --- a/package/MDAnalysis/guesser/base.py +++ b/package/MDAnalysis/guesser/base.py @@ -146,12 +146,12 @@ def guess_attr(self, attr_to_guess, force_guess=False): try: guesser_method = self._guesser_methods[attr_to_guess] except KeyError: - raise ValueError(f'{type(self).__name__} cannot guess the following ' + raise ValueError(f'{type(self).__name__} cannot guess this ' f'attribute: {attr_to_guess}') - + # make attribute to guess plural attr_to_guess = top_attr.attrname - + # Connection attributes should be just returned as they are always # appended to the Universe. ``force_guess`` handling should happen # at Universe level. From aed3d3108c6613651ee55e830c5264648dac9c47 Mon Sep 17 00:00:00 2001 From: Lily Wang Date: Fri, 25 Oct 2024 18:30:56 +1100 Subject: [PATCH 07/30] cleanup --- package/MDAnalysis/core/universe.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/package/MDAnalysis/core/universe.py b/package/MDAnalysis/core/universe.py index ed2c34354d..338f894874 100644 --- a/package/MDAnalysis/core/universe.py +++ b/package/MDAnalysis/core/universe.py @@ -1135,6 +1135,7 @@ def _add_topology_objects(self, object_type, values, types=None, guessed=False, self.add_TopologyAttr(object_type, []) attr = getattr(self._topology, object_type) + attr._add_bonds(indices, types=types, guessed=guessed, order=order) def add_bonds(self, values, types=None, guessed=False, order=None): @@ -1292,6 +1293,7 @@ def _delete_topology_objects(self, object_type, values): attr = getattr(self._topology, object_type) except AttributeError: raise ValueError('There are no {} to delete'.format(object_type)) + attr._delete_bonds(indices) def delete_bonds(self, values): @@ -1595,7 +1597,6 @@ def guess_TopologyAttrs( # delete existing connections if they exist if fg and hasattr(self.atoms, attr): group = getattr(self.atoms, attr) - self.delete_bonds(group) self._delete_topology_objects(attr, group) # this method appends to existing bonds self._add_topology_objects( From e88af7ab25bce3aa2aa3b818a65e7acc8446cfbe Mon Sep 17 00:00:00 2001 From: Lily Wang Date: Fri, 25 Oct 2024 18:50:21 +1100 Subject: [PATCH 08/30] add more tests --- .../MDAnalysisTests/guesser/test_base.py | 44 ++++++++++++++++++- 1 file changed, 43 insertions(+), 1 deletion(-) diff --git a/testsuite/MDAnalysisTests/guesser/test_base.py b/testsuite/MDAnalysisTests/guesser/test_base.py index 3e3c9bef51..6a9698391f 100644 --- a/testsuite/MDAnalysisTests/guesser/test_base.py +++ b/testsuite/MDAnalysisTests/guesser/test_base.py @@ -104,7 +104,26 @@ def test_partial_guess_attr_with_unknown_no_value_label(self): u = mda.Universe(top, to_guess=['types']) assert_equal(u.atoms.types, ['', '', '', '']) - def test_guess_topology_objects_existing(self): + def test_guess_topology_objects_existing_read(self): + u = mda.Universe(datafiles.CONECT) + assert len(u.atoms.bonds) == 72 + assert list(u.bonds[0].indices) == [623, 630] + + # delete some bonds + u.delete_bonds(u.atoms.bonds[:10]) + assert len(u.atoms.bonds) == 62 + assert list(u.bonds[0].indices) == [623, 630] + + all_indices = [tuple(x.indices) for x in u.bonds] + assert (0, 1) not in all_indices + + # guess old bonds back + u.guess_TopologyAttrs("default", to_guess=["bonds"]) + assert len(u.atoms.bonds) == 72 + # check TopologyGroup contains new (old) bonds + assert list(u.bonds[0].indices) == [623, 630] + + def test_guess_topology_objects_existing_in_universe(self): u = mda.Universe(datafiles.CONECT, to_guess=["bonds"]) assert len(u.atoms.bonds) == 1922 assert list(u.bonds[0].indices) == [0, 1] @@ -164,3 +183,26 @@ def test_force_guess_overwrites_existing_bonds(self): u.guess_TopologyAttrs("default", force_guess=["bonds"], vdwradii=vdw) assert len(u.atoms.bonds) == 0 assert False + + def test_guessing_angles_respects_bond_kwargs(self): + u = mda.Universe(datafiles.PDB) + assert not hasattr(u.atoms, "angles") + + # This low radius should find no angles + vdw = dict.fromkeys(set(u.atoms.types), 0.01) + + u.guess_TopologyAttrs("default", to_guess=["angles"], vdwradii=vdw) + assert len(u.atoms.angles) == 0 + + # set higher radii for lots of angles! + vdw = dict.fromkeys(set(u.atoms.types), 1) + u.guess_TopologyAttrs("default", force_guess=["angles"], vdwradii=vdw) + assert len(u.atoms.angles) == 89466 + + def test_guessing_dihedrals_respects_bond_kwargs(self): + u = mda.Universe(datafiles.CONECT) + assert len(u.atoms.bonds) == 72 + + u.guess_TopologyAttrs("default", to_guess=["dihedrals"]) + assert len(u.atoms.dihedrals) == 3548 + assert not hasattr(u.atoms, "angles") From 11a14c10b22a69a15462e8bef034e1c032be0920 Mon Sep 17 00:00:00 2001 From: Lily Wang Date: Fri, 25 Oct 2024 18:52:12 +1100 Subject: [PATCH 09/30] update changelog --- package/CHANGELOG | 3 +++ 1 file changed, 3 insertions(+) diff --git a/package/CHANGELOG b/package/CHANGELOG index 52e8eb077b..7be6db672e 100644 --- a/package/CHANGELOG +++ b/package/CHANGELOG @@ -23,6 +23,9 @@ The rules for this file: * 2.8.0 Fixes + * Allows bond/angle/dihedral connectivity to be guessed additively with + `to_guess`, and as a replacement of existing values with `force_guess`. + Also updates cached bond attributes when updating bonds. (Issue #4759, PR #4761) * Adds guessed attributes documentation back to each parser page and updates overall guesser docs (Issue #4696) * Fix Bohrium (Bh) atomic mass in tables.py (PR #3753) From 219ee09906fdf48fc9cb5b7334ae6daa62e60479 Mon Sep 17 00:00:00 2001 From: Lily Wang Date: Fri, 25 Oct 2024 16:31:23 +1100 Subject: [PATCH 10/30] add tests --- .../MDAnalysisTests/guesser/test_base.py | 48 +++++++++++++++++++ 1 file changed, 48 insertions(+) diff --git a/testsuite/MDAnalysisTests/guesser/test_base.py b/testsuite/MDAnalysisTests/guesser/test_base.py index d9c7ffc3e2..882d034378 100644 --- a/testsuite/MDAnalysisTests/guesser/test_base.py +++ b/testsuite/MDAnalysisTests/guesser/test_base.py @@ -230,3 +230,51 @@ def test_universe_creation_from_specific_array(): [0., 200., 150.], [100., 200., 150.], [200., 200., 150.] ]) mda.Universe(a, n_atoms=9) + def test_guess_topology_objects_existing(self): + u = mda.Universe(datafiles.CONECT, to_guess=["bonds"]) + assert len(u.atoms.bonds) == 1922 + assert list(u.bonds[0].indices) == [0, 1] + + # delete some bonds + u.delete_bonds(u.atoms.bonds[:100]) + assert len(u.atoms.bonds) == 1822 + assert list(u.bonds[0].indices) == [94, 99] + + all_indices = [tuple(x.indices) for x in u.bonds] + assert (0, 1) not in all_indices + + # guess old bonds back + u.guess_TopologyAttrs("default", to_guess=["bonds"]) + assert len(u.atoms.bonds) == 1922 + # check TopologyGroup contains new (old) bonds + assert list(u.bonds[0].indices) == [0, 1] + + def test_guess_topology_objects_force(self): + u = mda.Universe(datafiles.CONECT, force_guess=["bonds"]) + assert len(u.atoms.bonds) == 1922 + + with pytest.raises(NoDataError): + u.atoms.angles + + + def test_guess_topology_objects_out_of_order_init(self): + u = mda.Universe( + datafiles.PDB_small, + to_guess=["dihedrals", "angles", "bonds"], + guess_bonds=False + ) + assert len(u.atoms.angles) == 6123 + assert len(u.atoms.dihedrals) == 8921 + + def test_guess_topology_objects_out_of_order_guess(self): + u = mda.Universe(datafiles.PDB_small) + with pytest.raises(NoDataError): + u.atoms.angles + + u.guess_TopologyAttrs( + "default", + to_guess=["dihedrals", "angles", "bonds"] + ) + assert len(u.atoms.angles) == 6123 + assert len(u.atoms.dihedrals) == 8921 + From 1e6d9ab93edefc894eb779a0c4f56f8f949bc055 Mon Sep 17 00:00:00 2001 From: Lily Wang Date: Fri, 25 Oct 2024 18:10:27 +1100 Subject: [PATCH 11/30] add force guessing removal --- package/MDAnalysis/core/universe.py | 8 ++-- package/MDAnalysis/guesser/base.py | 9 ++++ .../MDAnalysisTests/guesser/test_base.py | 48 +++++++++---------- 3 files changed, 38 insertions(+), 27 deletions(-) diff --git a/package/MDAnalysis/core/universe.py b/package/MDAnalysis/core/universe.py index 7b635788bf..00b5e2e2f3 100644 --- a/package/MDAnalysis/core/universe.py +++ b/package/MDAnalysis/core/universe.py @@ -1183,7 +1183,6 @@ def _add_topology_objects(self, object_type, values, types=None, guessed=False, self.add_TopologyAttr(object_type, []) attr = getattr(self._topology, object_type) - attr._add_bonds(indices, types=types, guessed=guessed, order=order) def add_bonds(self, values, types=None, guessed=False, order=None): @@ -1235,7 +1234,7 @@ def add_bonds(self, values, types=None, guessed=False, order=None): self._add_topology_objects('bonds', values, types=types, guessed=guessed, order=order) self._invalidate_bond_related_caches() - + def _invalidate_bond_related_caches(self): """ Invalidate caches related to bonds and fragments. @@ -1341,7 +1340,6 @@ def _delete_topology_objects(self, object_type, values): attr = getattr(self._topology, object_type) except AttributeError: raise ValueError('There are no {} to delete'.format(object_type)) - attr._delete_bonds(indices) def delete_bonds(self, values): @@ -1665,6 +1663,10 @@ def guess_TopologyAttrs( # delete existing connections if they exist if fg and hasattr(self.atoms, attr): group = getattr(self.atoms, attr) +<<<<<<< HEAD +======= + self.delete_bonds(group) +>>>>>>> ed1ba84de (add force guessing removal) self._delete_topology_objects(attr, group) # this method appends to existing bonds self._add_topology_objects( diff --git a/package/MDAnalysis/guesser/base.py b/package/MDAnalysis/guesser/base.py index 7360df7b43..5ba4d731bf 100644 --- a/package/MDAnalysis/guesser/base.py +++ b/package/MDAnalysis/guesser/base.py @@ -148,6 +148,15 @@ def guess_attr(self, attr_to_guess, force_guess=False): except KeyError: raise ValueError(f'{type(self).__name__} cannot guess this ' f'attribute: {attr_to_guess}') + + # make attribute to guess plural + attr_to_guess = top_attr.attrname + + # Connection attributes should be just returned as they are always + # appended to the Universe. ``force_guess`` handling should happen + # at Universe level. + if issubclass(top_attr, _Connection): + return guesser_method() # make attribute to guess plural attr_to_guess = top_attr.attrname diff --git a/testsuite/MDAnalysisTests/guesser/test_base.py b/testsuite/MDAnalysisTests/guesser/test_base.py index 882d034378..16637eebdc 100644 --- a/testsuite/MDAnalysisTests/guesser/test_base.py +++ b/testsuite/MDAnalysisTests/guesser/test_base.py @@ -207,29 +207,6 @@ def test_guessing_dihedrals_respects_bond_kwargs(self): assert len(u.atoms.dihedrals) == 3548 assert not hasattr(u.atoms, "angles") -def test_Universe_guess_bonds_deprecated(): - with pytest.warns( - DeprecationWarning, - match='`guess_bonds` keyword is deprecated' - ): - u = mda.Universe(datafiles.PDB_full, guess_bonds=True) - - -@pytest.mark.parametrize( - "universe_input", - [datafiles.DCD, datafiles.XTC, np.random.rand(3, 3), datafiles.PDB] -) -def test_universe_creation_from_coordinates(universe_input): - mda.Universe(universe_input) - - -def test_universe_creation_from_specific_array(): - a = np.array([ - [0., 0., 150.], [0., 0., 150.], [200., 0., 150.], - [0., 0., 150.], [100., 100., 150.], [200., 100., 150.], - [0., 200., 150.], [100., 200., 150.], [200., 200., 150.] - ]) - mda.Universe(a, n_atoms=9) def test_guess_topology_objects_existing(self): u = mda.Universe(datafiles.CONECT, to_guess=["bonds"]) assert len(u.atoms.bonds) == 1922 @@ -256,7 +233,6 @@ def test_guess_topology_objects_force(self): with pytest.raises(NoDataError): u.atoms.angles - def test_guess_topology_objects_out_of_order_init(self): u = mda.Universe( datafiles.PDB_small, @@ -278,3 +254,27 @@ def test_guess_topology_objects_out_of_order_guess(self): assert len(u.atoms.angles) == 6123 assert len(u.atoms.dihedrals) == 8921 + +def test_Universe_guess_bonds_deprecated(): + with pytest.warns( + DeprecationWarning, + match='`guess_bonds` keyword is deprecated' + ): + u = mda.Universe(datafiles.PDB_full, guess_bonds=True) + + +@pytest.mark.parametrize( + "universe_input", + [datafiles.DCD, datafiles.XTC, np.random.rand(3, 3), datafiles.PDB] +) +def test_universe_creation_from_coordinates(universe_input): + mda.Universe(universe_input) + + +def test_universe_creation_from_specific_array(): + a = np.array([ + [0., 0., 150.], [0., 0., 150.], [200., 0., 150.], + [0., 0., 150.], [100., 100., 150.], [200., 100., 150.], + [0., 200., 150.], [100., 200., 150.], [200., 200., 150.] + ]) + mda.Universe(a, n_atoms=9) \ No newline at end of file From 2918ae7da2b5b2c4d070480ac8cc301420e44602 Mon Sep 17 00:00:00 2001 From: Lily Wang Date: Fri, 25 Oct 2024 18:50:21 +1100 Subject: [PATCH 12/30] add more tests --- .../MDAnalysisTests/guesser/test_base.py | 22 +++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/testsuite/MDAnalysisTests/guesser/test_base.py b/testsuite/MDAnalysisTests/guesser/test_base.py index 16637eebdc..4995baa36e 100644 --- a/testsuite/MDAnalysisTests/guesser/test_base.py +++ b/testsuite/MDAnalysisTests/guesser/test_base.py @@ -109,6 +109,7 @@ def test_guess_topology_objects_existing_read(self): assert len(u.atoms.bonds) == 72 assert list(u.bonds[0].indices) == [623, 630] +<<<<<<< HEAD # delete some bonds u.delete_bonds(u.atoms.bonds[:10]) assert len(u.atoms.bonds) == 62 @@ -123,6 +124,27 @@ def test_guess_topology_objects_existing_read(self): # check TopologyGroup contains new (old) bonds assert list(u.bonds[0].indices) == [623, 630] +======= + def test_guess_topology_objects_existing_read(self): + u = mda.Universe(datafiles.CONECT) + assert len(u.atoms.bonds) == 72 + assert list(u.bonds[0].indices) == [623, 630] + + # delete some bonds + u.delete_bonds(u.atoms.bonds[:10]) + assert len(u.atoms.bonds) == 62 + assert list(u.bonds[0].indices) == [623, 630] + + all_indices = [tuple(x.indices) for x in u.bonds] + assert (0, 1) not in all_indices + + # guess old bonds back + u.guess_TopologyAttrs("default", to_guess=["bonds"]) + assert len(u.atoms.bonds) == 72 + # check TopologyGroup contains new (old) bonds + assert list(u.bonds[0].indices) == [623, 630] + +>>>>>>> 7236e9112 (add more tests) def test_guess_topology_objects_existing_in_universe(self): u = mda.Universe(datafiles.CONECT, to_guess=["bonds"]) assert len(u.atoms.bonds) == 1922 From 3e170077f2919986e0d591a2d2d6f59d5ba0addc Mon Sep 17 00:00:00 2001 From: Lily Wang Date: Sun, 27 Oct 2024 01:19:41 +1100 Subject: [PATCH 13/30] update tests after deletion fix --- testsuite/MDAnalysisTests/guesser/test_base.py | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/testsuite/MDAnalysisTests/guesser/test_base.py b/testsuite/MDAnalysisTests/guesser/test_base.py index 4995baa36e..0c9415e824 100644 --- a/testsuite/MDAnalysisTests/guesser/test_base.py +++ b/testsuite/MDAnalysisTests/guesser/test_base.py @@ -133,16 +133,16 @@ def test_guess_topology_objects_existing_read(self): # delete some bonds u.delete_bonds(u.atoms.bonds[:10]) assert len(u.atoms.bonds) == 62 - assert list(u.bonds[0].indices) == [623, 630] + assert list(u.bonds[0].indices) == [1545, 1552] # first bond has changed all_indices = [tuple(x.indices) for x in u.bonds] - assert (0, 1) not in all_indices + assert (623, 630) not in all_indices - # guess old bonds back + # test guessing new bonds doesn't remove old ones u.guess_TopologyAttrs("default", to_guess=["bonds"]) - assert len(u.atoms.bonds) == 72 - # check TopologyGroup contains new (old) bonds - assert list(u.bonds[0].indices) == [623, 630] + assert len(u.atoms.bonds) == 1922 + all_indices = [tuple(x.indices) for x in u.bonds] + assert (1545, 1552) in all_indices >>>>>>> 7236e9112 (add more tests) def test_guess_topology_objects_existing_in_universe(self): @@ -204,7 +204,6 @@ def test_force_guess_overwrites_existing_bonds(self): # Now force guess bonds u.guess_TopologyAttrs("default", force_guess=["bonds"], vdwradii=vdw) assert len(u.atoms.bonds) == 0 - assert False def test_guessing_angles_respects_bond_kwargs(self): u = mda.Universe(datafiles.PDB) From 892bc42ef22c27eb9698a47624c29d04f89a499f Mon Sep 17 00:00:00 2001 From: Lily Wang Date: Sun, 27 Oct 2024 01:23:49 +1100 Subject: [PATCH 14/30] fix problematic merge --- .../MDAnalysisTests/guesser/test_base.py | 22 ------------------- 1 file changed, 22 deletions(-) diff --git a/testsuite/MDAnalysisTests/guesser/test_base.py b/testsuite/MDAnalysisTests/guesser/test_base.py index 0c9415e824..e3c9553089 100644 --- a/testsuite/MDAnalysisTests/guesser/test_base.py +++ b/testsuite/MDAnalysisTests/guesser/test_base.py @@ -109,27 +109,6 @@ def test_guess_topology_objects_existing_read(self): assert len(u.atoms.bonds) == 72 assert list(u.bonds[0].indices) == [623, 630] -<<<<<<< HEAD - # delete some bonds - u.delete_bonds(u.atoms.bonds[:10]) - assert len(u.atoms.bonds) == 62 - assert list(u.bonds[0].indices) == [623, 630] - - all_indices = [tuple(x.indices) for x in u.bonds] - assert (0, 1) not in all_indices - - # guess old bonds back - u.guess_TopologyAttrs("default", to_guess=["bonds"]) - assert len(u.atoms.bonds) == 72 - # check TopologyGroup contains new (old) bonds - assert list(u.bonds[0].indices) == [623, 630] - -======= - def test_guess_topology_objects_existing_read(self): - u = mda.Universe(datafiles.CONECT) - assert len(u.atoms.bonds) == 72 - assert list(u.bonds[0].indices) == [623, 630] - # delete some bonds u.delete_bonds(u.atoms.bonds[:10]) assert len(u.atoms.bonds) == 62 @@ -144,7 +123,6 @@ def test_guess_topology_objects_existing_read(self): all_indices = [tuple(x.indices) for x in u.bonds] assert (1545, 1552) in all_indices ->>>>>>> 7236e9112 (add more tests) def test_guess_topology_objects_existing_in_universe(self): u = mda.Universe(datafiles.CONECT, to_guess=["bonds"]) assert len(u.atoms.bonds) == 1922 From 451d6a82fbd2217367369cb02dc14c60ddfc43c7 Mon Sep 17 00:00:00 2001 From: Lily Wang Date: Sun, 27 Oct 2024 01:24:45 +1100 Subject: [PATCH 15/30] fix other merge problem --- package/MDAnalysis/core/universe.py | 4 ---- 1 file changed, 4 deletions(-) diff --git a/package/MDAnalysis/core/universe.py b/package/MDAnalysis/core/universe.py index 00b5e2e2f3..16ffc1dc42 100644 --- a/package/MDAnalysis/core/universe.py +++ b/package/MDAnalysis/core/universe.py @@ -1663,10 +1663,6 @@ def guess_TopologyAttrs( # delete existing connections if they exist if fg and hasattr(self.atoms, attr): group = getattr(self.atoms, attr) -<<<<<<< HEAD -======= - self.delete_bonds(group) ->>>>>>> ed1ba84de (add force guessing removal) self._delete_topology_objects(attr, group) # this method appends to existing bonds self._add_topology_objects( From f044fa1d3787c7eb634ab5a6e6de778be294ef5f Mon Sep 17 00:00:00 2001 From: Lily Wang Date: Sun, 27 Oct 2024 01:26:34 +1100 Subject: [PATCH 16/30] how did this rebase go so wrong --- .../MDAnalysisTests/guesser/test_base.py | 47 ------------------- 1 file changed, 47 deletions(-) diff --git a/testsuite/MDAnalysisTests/guesser/test_base.py b/testsuite/MDAnalysisTests/guesser/test_base.py index e3c9553089..6e43c41e70 100644 --- a/testsuite/MDAnalysisTests/guesser/test_base.py +++ b/testsuite/MDAnalysisTests/guesser/test_base.py @@ -206,53 +206,6 @@ def test_guessing_dihedrals_respects_bond_kwargs(self): assert len(u.atoms.dihedrals) == 3548 assert not hasattr(u.atoms, "angles") - def test_guess_topology_objects_existing(self): - u = mda.Universe(datafiles.CONECT, to_guess=["bonds"]) - assert len(u.atoms.bonds) == 1922 - assert list(u.bonds[0].indices) == [0, 1] - - # delete some bonds - u.delete_bonds(u.atoms.bonds[:100]) - assert len(u.atoms.bonds) == 1822 - assert list(u.bonds[0].indices) == [94, 99] - - all_indices = [tuple(x.indices) for x in u.bonds] - assert (0, 1) not in all_indices - - # guess old bonds back - u.guess_TopologyAttrs("default", to_guess=["bonds"]) - assert len(u.atoms.bonds) == 1922 - # check TopologyGroup contains new (old) bonds - assert list(u.bonds[0].indices) == [0, 1] - - def test_guess_topology_objects_force(self): - u = mda.Universe(datafiles.CONECT, force_guess=["bonds"]) - assert len(u.atoms.bonds) == 1922 - - with pytest.raises(NoDataError): - u.atoms.angles - - def test_guess_topology_objects_out_of_order_init(self): - u = mda.Universe( - datafiles.PDB_small, - to_guess=["dihedrals", "angles", "bonds"], - guess_bonds=False - ) - assert len(u.atoms.angles) == 6123 - assert len(u.atoms.dihedrals) == 8921 - - def test_guess_topology_objects_out_of_order_guess(self): - u = mda.Universe(datafiles.PDB_small) - with pytest.raises(NoDataError): - u.atoms.angles - - u.guess_TopologyAttrs( - "default", - to_guess=["dihedrals", "angles", "bonds"] - ) - assert len(u.atoms.angles) == 6123 - assert len(u.atoms.dihedrals) == 8921 - def test_Universe_guess_bonds_deprecated(): with pytest.warns( From e17c57ef19a5b6868e644fa4840702fce1fe3e42 Mon Sep 17 00:00:00 2001 From: Lily Wang Date: Sun, 27 Oct 2024 01:28:08 +1100 Subject: [PATCH 17/30] repep8 --- package/MDAnalysis/core/universe.py | 2 +- package/MDAnalysis/guesser/base.py | 4 ++-- testsuite/MDAnalysisTests/guesser/test_base.py | 5 +++-- 3 files changed, 6 insertions(+), 5 deletions(-) diff --git a/package/MDAnalysis/core/universe.py b/package/MDAnalysis/core/universe.py index 16ffc1dc42..69b5fe0ef9 100644 --- a/package/MDAnalysis/core/universe.py +++ b/package/MDAnalysis/core/universe.py @@ -1234,7 +1234,7 @@ def add_bonds(self, values, types=None, guessed=False, order=None): self._add_topology_objects('bonds', values, types=types, guessed=guessed, order=order) self._invalidate_bond_related_caches() - + def _invalidate_bond_related_caches(self): """ Invalidate caches related to bonds and fragments. diff --git a/package/MDAnalysis/guesser/base.py b/package/MDAnalysis/guesser/base.py index 5ba4d731bf..bb526914e3 100644 --- a/package/MDAnalysis/guesser/base.py +++ b/package/MDAnalysis/guesser/base.py @@ -148,10 +148,10 @@ def guess_attr(self, attr_to_guess, force_guess=False): except KeyError: raise ValueError(f'{type(self).__name__} cannot guess this ' f'attribute: {attr_to_guess}') - + # make attribute to guess plural attr_to_guess = top_attr.attrname - + # Connection attributes should be just returned as they are always # appended to the Universe. ``force_guess`` handling should happen # at Universe level. diff --git a/testsuite/MDAnalysisTests/guesser/test_base.py b/testsuite/MDAnalysisTests/guesser/test_base.py index 6e43c41e70..3c425ae319 100644 --- a/testsuite/MDAnalysisTests/guesser/test_base.py +++ b/testsuite/MDAnalysisTests/guesser/test_base.py @@ -112,7 +112,8 @@ def test_guess_topology_objects_existing_read(self): # delete some bonds u.delete_bonds(u.atoms.bonds[:10]) assert len(u.atoms.bonds) == 62 - assert list(u.bonds[0].indices) == [1545, 1552] # first bond has changed + # first bond has changed + assert list(u.bonds[0].indices) == [1545, 1552] all_indices = [tuple(x.indices) for x in u.bonds] assert (623, 630) not in all_indices @@ -229,4 +230,4 @@ def test_universe_creation_from_specific_array(): [0., 0., 150.], [100., 100., 150.], [200., 100., 150.], [0., 200., 150.], [100., 200., 150.], [200., 200., 150.] ]) - mda.Universe(a, n_atoms=9) \ No newline at end of file + mda.Universe(a, n_atoms=9) From 1fc4514bfb8b6bd74ae0bbb3075b2a9097171d9e Mon Sep 17 00:00:00 2001 From: Lily Wang Date: Sun, 27 Oct 2024 01:33:49 +1100 Subject: [PATCH 18/30] more merge fixes --- package/MDAnalysis/guesser/base.py | 9 --------- 1 file changed, 9 deletions(-) diff --git a/package/MDAnalysis/guesser/base.py b/package/MDAnalysis/guesser/base.py index bb526914e3..7360df7b43 100644 --- a/package/MDAnalysis/guesser/base.py +++ b/package/MDAnalysis/guesser/base.py @@ -152,15 +152,6 @@ def guess_attr(self, attr_to_guess, force_guess=False): # make attribute to guess plural attr_to_guess = top_attr.attrname - # Connection attributes should be just returned as they are always - # appended to the Universe. ``force_guess`` handling should happen - # at Universe level. - if issubclass(top_attr, _Connection): - return guesser_method() - - # make attribute to guess plural - attr_to_guess = top_attr.attrname - # Connection attributes should be just returned as they are always # appended to the Universe. ``force_guess`` handling should happen # at Universe level. From 7305e60340eff2a38ed3dde4f6754c44df6370df Mon Sep 17 00:00:00 2001 From: Lily Wang Date: Sun, 27 Oct 2024 02:02:09 +1100 Subject: [PATCH 19/30] change guessing bonds with force to additive --- package/MDAnalysis/core/universe.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package/MDAnalysis/core/universe.py b/package/MDAnalysis/core/universe.py index 69b5fe0ef9..101f2d37db 100644 --- a/package/MDAnalysis/core/universe.py +++ b/package/MDAnalysis/core/universe.py @@ -457,7 +457,7 @@ def __init__(self, topology=None, *coordinates, all_coordinates=False, "the previous Context values.", DeprecationWarning ) - force_guess = list(force_guess) + ['bonds', 'angles', 'dihedrals'] + to_guess = list(to_guess) + ['bonds', 'angles', 'dihedrals'] self.guess_TopologyAttrs( context, to_guess, force_guess, error_if_missing=False From 970371fd4c2e1d49b312736bd45750a913fdf15d Mon Sep 17 00:00:00 2001 From: Lily Wang Date: Sun, 27 Oct 2024 02:38:05 +1100 Subject: [PATCH 20/30] change valueerror to nodataerror --- package/MDAnalysis/core/universe.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package/MDAnalysis/core/universe.py b/package/MDAnalysis/core/universe.py index 101f2d37db..a9046be103 100644 --- a/package/MDAnalysis/core/universe.py +++ b/package/MDAnalysis/core/universe.py @@ -1649,7 +1649,7 @@ def guess_TopologyAttrs( fg = attr in force_guess try: values = guesser.guess_attr(attr, fg) - except ValueError as e: + except NoDataError as e: if error_if_missing or fg: raise e else: From 909ec37dc28bf65b65990802f5967ae3a8d01825 Mon Sep 17 00:00:00 2001 From: Lily Wang Date: Sun, 27 Oct 2024 12:41:41 +1100 Subject: [PATCH 21/30] add tests for errors --- package/MDAnalysis/guesser/base.py | 6 ++--- .../MDAnalysisTests/guesser/test_base.py | 22 +++++++++++++++++++ 2 files changed, 25 insertions(+), 3 deletions(-) diff --git a/package/MDAnalysis/guesser/base.py b/package/MDAnalysis/guesser/base.py index 7360df7b43..0fd7a7e18e 100644 --- a/package/MDAnalysis/guesser/base.py +++ b/package/MDAnalysis/guesser/base.py @@ -143,15 +143,15 @@ def guess_attr(self, attr_to_guess, force_guess=False): f"{attr_to_guess} is not a recognized MDAnalysis " "topology attribute" ) + # make attribute to guess plural + attr_to_guess = top_attr.attrname + try: guesser_method = self._guesser_methods[attr_to_guess] except KeyError: raise ValueError(f'{type(self).__name__} cannot guess this ' f'attribute: {attr_to_guess}') - # make attribute to guess plural - attr_to_guess = top_attr.attrname - # Connection attributes should be just returned as they are always # appended to the Universe. ``force_guess`` handling should happen # at Universe level. diff --git a/testsuite/MDAnalysisTests/guesser/test_base.py b/testsuite/MDAnalysisTests/guesser/test_base.py index 3c425ae319..8c80bed4b6 100644 --- a/testsuite/MDAnalysisTests/guesser/test_base.py +++ b/testsuite/MDAnalysisTests/guesser/test_base.py @@ -207,6 +207,28 @@ def test_guessing_dihedrals_respects_bond_kwargs(self): assert len(u.atoms.dihedrals) == 3548 assert not hasattr(u.atoms, "angles") + def test_guess_invalid_attribute(self): + default_guesser = get_guesser("default") + err = "not a recognized MDAnalysis topology attribute" + with pytest.raises(KeyError, match=err): + default_guesser.guess_attr('not_an_attribute') + + def test_guess_unsupported_attribute(self): + default_guesser = get_guesser("default") + err = "cannot guess this attribute" + with pytest.raises(ValueError, match=err): + default_guesser.guess_attr('tempfactors') + + def test_guess_singular(self): + default_guesser = get_guesser("default") + u = mda.Universe(datafiles.PDB, to_guess=[]) + assert not hasattr(u.atoms, "masses") + + default_guesser._universe = u + masses = default_guesser.guess_attr('mass') + + + def test_Universe_guess_bonds_deprecated(): with pytest.warns( From c190ca8c41bedff979fbd6bc97d986a4d5252ff9 Mon Sep 17 00:00:00 2001 From: Lily Wang <31115101+lilyminium@users.noreply.github.com> Date: Tue, 29 Oct 2024 06:28:23 +1100 Subject: [PATCH 22/30] Update package/MDAnalysis/core/universe.py Co-authored-by: Irfan Alibay --- package/MDAnalysis/core/universe.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/package/MDAnalysis/core/universe.py b/package/MDAnalysis/core/universe.py index a9046be103..467a2f8e7f 100644 --- a/package/MDAnalysis/core/universe.py +++ b/package/MDAnalysis/core/universe.py @@ -457,6 +457,9 @@ def __init__(self, topology=None, *coordinates, all_coordinates=False, "the previous Context values.", DeprecationWarning ) + # Original behaviour is to add additionally guessed bond info + # this is achieved by adding to the `to_guess` list (unliked `force_guess` + # which replaces existing bonds). to_guess = list(to_guess) + ['bonds', 'angles', 'dihedrals'] self.guess_TopologyAttrs( From f03a39425c07cc8151f399013ecbe5f789a03a39 Mon Sep 17 00:00:00 2001 From: Lily Wang <31115101+lilyminium@users.noreply.github.com> Date: Tue, 29 Oct 2024 06:28:39 +1100 Subject: [PATCH 23/30] Update package/MDAnalysis/core/universe.py Co-authored-by: Irfan Alibay --- package/MDAnalysis/core/universe.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/package/MDAnalysis/core/universe.py b/package/MDAnalysis/core/universe.py index 467a2f8e7f..090078fd35 100644 --- a/package/MDAnalysis/core/universe.py +++ b/package/MDAnalysis/core/universe.py @@ -1241,6 +1241,8 @@ def add_bonds(self, values, types=None, guessed=False, order=None): def _invalidate_bond_related_caches(self): """ Invalidate caches related to bonds and fragments. + + This should be called whenever the Universe's bonds are modified. .. versionadded: 2.8.0 """ From 80c68a6ec5873cdbfe57df07b2cf5dc118abb4b7 Mon Sep 17 00:00:00 2001 From: Lily Wang <31115101+lilyminium@users.noreply.github.com> Date: Tue, 29 Oct 2024 06:28:52 +1100 Subject: [PATCH 24/30] Update package/MDAnalysis/core/universe.py Co-authored-by: Irfan Alibay --- package/MDAnalysis/core/universe.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/package/MDAnalysis/core/universe.py b/package/MDAnalysis/core/universe.py index 090078fd35..2b8b3aa605 100644 --- a/package/MDAnalysis/core/universe.py +++ b/package/MDAnalysis/core/universe.py @@ -1624,6 +1624,8 @@ def guess_TopologyAttrs( # in the same order that the user provided total_guess = list(dict.fromkeys(total_guess)) + # Set of all Connectivity related attribute names + # used to special case attribute replacement after calling the guesser objects = set( topattr.attrname for topattr in _TOPOLOGY_ATTRS.values() if issubclass(topattr, _Connection) From 5ab97d4beac994f4c0a6004150a80da356eadb4f Mon Sep 17 00:00:00 2001 From: Lily Wang <31115101+lilyminium@users.noreply.github.com> Date: Tue, 29 Oct 2024 06:34:05 +1100 Subject: [PATCH 25/30] Update package/MDAnalysis/core/universe.py Co-authored-by: Irfan Alibay --- package/MDAnalysis/core/universe.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package/MDAnalysis/core/universe.py b/package/MDAnalysis/core/universe.py index 2b8b3aa605..a2bc60c25f 100644 --- a/package/MDAnalysis/core/universe.py +++ b/package/MDAnalysis/core/universe.py @@ -1671,7 +1671,7 @@ def guess_TopologyAttrs( if fg and hasattr(self.atoms, attr): group = getattr(self.atoms, attr) self._delete_topology_objects(attr, group) - # this method appends to existing bonds + # this method appends any new bonds in values to existing bonds self._add_topology_objects( attr, values, guessed=True) if attr == "bonds": From 38077d6e9a545688388a542af7575ad2351e5503 Mon Sep 17 00:00:00 2001 From: Lily Wang Date: Tue, 29 Oct 2024 06:44:43 +1100 Subject: [PATCH 26/30] add new test --- testsuite/MDAnalysisTests/guesser/test_base.py | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/testsuite/MDAnalysisTests/guesser/test_base.py b/testsuite/MDAnalysisTests/guesser/test_base.py index 8c80bed4b6..e3e7e340a5 100644 --- a/testsuite/MDAnalysisTests/guesser/test_base.py +++ b/testsuite/MDAnalysisTests/guesser/test_base.py @@ -114,6 +114,10 @@ def test_guess_topology_objects_existing_read(self): assert len(u.atoms.bonds) == 62 # first bond has changed assert list(u.bonds[0].indices) == [1545, 1552] + # count number of (1545, 1552) bonds + bond_values = u._topology.bonds.values + n_bonds = sum(1 for bond in bond_values if bond == (1545, 1552)) + assert n_bonds == 2 all_indices = [tuple(x.indices) for x in u.bonds] assert (623, 630) not in all_indices @@ -124,6 +128,11 @@ def test_guess_topology_objects_existing_read(self): all_indices = [tuple(x.indices) for x in u.bonds] assert (1545, 1552) in all_indices + # test guessing new bonds doesn't duplicate old ones + bond_values = u._topology.bonds.values + n_bonds = sum(1 for bond in bond_values if bond == (1545, 1552)) + assert n_bonds == 2 + def test_guess_topology_objects_existing_in_universe(self): u = mda.Universe(datafiles.CONECT, to_guess=["bonds"]) assert len(u.atoms.bonds) == 1922 From ab6418e23d874b3c6e3aa43b87ebb5d2c83ab26c Mon Sep 17 00:00:00 2001 From: Lily Wang Date: Tue, 29 Oct 2024 06:57:16 +1100 Subject: [PATCH 27/30] add guessing check --- package/MDAnalysis/core/topologyattrs.py | 1 + testsuite/MDAnalysisTests/guesser/test_base.py | 12 ++++++++---- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/package/MDAnalysis/core/topologyattrs.py b/package/MDAnalysis/core/topologyattrs.py index ef5897268c..d4310e472e 100644 --- a/package/MDAnalysis/core/topologyattrs.py +++ b/package/MDAnalysis/core/topologyattrs.py @@ -3129,6 +3129,7 @@ def _add_bonds(self, values, types=None, guessed=True, order=None): del self._cache['bd'] except KeyError: pass + print(self._guessed[69], self._guessed[100]) @_check_connection_values def _delete_bonds(self, values): diff --git a/testsuite/MDAnalysisTests/guesser/test_base.py b/testsuite/MDAnalysisTests/guesser/test_base.py index e3e7e340a5..a56003500c 100644 --- a/testsuite/MDAnalysisTests/guesser/test_base.py +++ b/testsuite/MDAnalysisTests/guesser/test_base.py @@ -116,8 +116,10 @@ def test_guess_topology_objects_existing_read(self): assert list(u.bonds[0].indices) == [1545, 1552] # count number of (1545, 1552) bonds bond_values = u._topology.bonds.values - n_bonds = sum(1 for bond in bond_values if bond == (1545, 1552)) - assert n_bonds == 2 + bond_indices = [i for i, x in enumerate(bond_values) if x == (1545, 1552)] + for i in bond_indices: + assert u._topology.bonds._guessed[i] == False + assert len(bond_indices) == 2 all_indices = [tuple(x.indices) for x in u.bonds] assert (623, 630) not in all_indices @@ -130,8 +132,10 @@ def test_guess_topology_objects_existing_read(self): # test guessing new bonds doesn't duplicate old ones bond_values = u._topology.bonds.values - n_bonds = sum(1 for bond in bond_values if bond == (1545, 1552)) - assert n_bonds == 2 + bond_indices = [i for i, x in enumerate(bond_values) if x == (1545, 1552)] + for i in bond_indices: + assert u._topology.bonds._guessed[i] == False + assert len(bond_indices) == 2 def test_guess_topology_objects_existing_in_universe(self): u = mda.Universe(datafiles.CONECT, to_guess=["bonds"]) From 26f91b5777a36660ce24679cb95e046744a238bf Mon Sep 17 00:00:00 2001 From: Lily Wang Date: Tue, 29 Oct 2024 06:59:50 +1100 Subject: [PATCH 28/30] oopsie --- package/MDAnalysis/core/topologyattrs.py | 1 - 1 file changed, 1 deletion(-) diff --git a/package/MDAnalysis/core/topologyattrs.py b/package/MDAnalysis/core/topologyattrs.py index d4310e472e..ef5897268c 100644 --- a/package/MDAnalysis/core/topologyattrs.py +++ b/package/MDAnalysis/core/topologyattrs.py @@ -3129,7 +3129,6 @@ def _add_bonds(self, values, types=None, guessed=True, order=None): del self._cache['bd'] except KeyError: pass - print(self._guessed[69], self._guessed[100]) @_check_connection_values def _delete_bonds(self, values): From 36b62e659c4416c912d12dfd8c0c4acd22de75c2 Mon Sep 17 00:00:00 2001 From: Lily Wang Date: Wed, 30 Oct 2024 00:58:51 +1100 Subject: [PATCH 29/30] update test --- .../MDAnalysisTests/guesser/test_base.py | 25 +++++++++---------- 1 file changed, 12 insertions(+), 13 deletions(-) diff --git a/testsuite/MDAnalysisTests/guesser/test_base.py b/testsuite/MDAnalysisTests/guesser/test_base.py index a56003500c..7edb18ef28 100644 --- a/testsuite/MDAnalysisTests/guesser/test_base.py +++ b/testsuite/MDAnalysisTests/guesser/test_base.py @@ -115,11 +115,10 @@ def test_guess_topology_objects_existing_read(self): # first bond has changed assert list(u.bonds[0].indices) == [1545, 1552] # count number of (1545, 1552) bonds - bond_values = u._topology.bonds.values - bond_indices = [i for i, x in enumerate(bond_values) if x == (1545, 1552)] - for i in bond_indices: - assert u._topology.bonds._guessed[i] == False - assert len(bond_indices) == 2 + ag = u.atoms[[1545, 1552]] + bonds = ag.bonds.atomgroup_intersection(ag, strict=True) + assert len(bonds) == 1 + assert not bonds[0].is_guessed all_indices = [tuple(x.indices) for x in u.bonds] assert (623, 630) not in all_indices @@ -127,15 +126,15 @@ def test_guess_topology_objects_existing_read(self): # test guessing new bonds doesn't remove old ones u.guess_TopologyAttrs("default", to_guess=["bonds"]) assert len(u.atoms.bonds) == 1922 - all_indices = [tuple(x.indices) for x in u.bonds] - assert (1545, 1552) in all_indices - + old_bonds = ag.bonds.atomgroup_intersection(ag, strict=True) + assert len(old_bonds) == 1 # test guessing new bonds doesn't duplicate old ones - bond_values = u._topology.bonds.values - bond_indices = [i for i, x in enumerate(bond_values) if x == (1545, 1552)] - for i in bond_indices: - assert u._topology.bonds._guessed[i] == False - assert len(bond_indices) == 2 + assert not old_bonds[0].is_guessed + + new_ag = u.atoms[[623, 630]] + new_bonds = new_ag.bonds.atomgroup_intersection(new_ag, strict=True) + assert len(new_bonds) == 1 + assert new_bonds[0].is_guessed def test_guess_topology_objects_existing_in_universe(self): u = mda.Universe(datafiles.CONECT, to_guess=["bonds"]) From 839e7f383e6a40a58c0d9d3c3f0a504d4632cb27 Mon Sep 17 00:00:00 2001 From: Irfan Alibay Date: Mon, 11 Nov 2024 08:38:22 +0000 Subject: [PATCH 30/30] Update testsuite/MDAnalysisTests/guesser/test_base.py --- testsuite/MDAnalysisTests/guesser/test_base.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/testsuite/MDAnalysisTests/guesser/test_base.py b/testsuite/MDAnalysisTests/guesser/test_base.py index 7edb18ef28..c44fdc3c59 100644 --- a/testsuite/MDAnalysisTests/guesser/test_base.py +++ b/testsuite/MDAnalysisTests/guesser/test_base.py @@ -240,8 +240,6 @@ def test_guess_singular(self): masses = default_guesser.guess_attr('mass') - - def test_Universe_guess_bonds_deprecated(): with pytest.warns( DeprecationWarning,