Fix: modernise HBA to use AG as objects internally instead of selection strings - code only PR (Issue #3933)

package/AUTHORS

@@ -240,8 +240,7 @@ Chronological list of authors
   - Sampurna Mukherjee
   - Leon Wehrhan
   - Valerij Talagayev
-
-
+  - Luna Morrow
 
 External code
 -------------

package/CHANGELOG

@@ -14,6 +14,27 @@ The rules for this file:
 
 
 -------------------------------------------------------------------------------
+04/25/24 lunamorrow
+
+ * 2.8.1
+
+Fixes
+
+Enhancements
+  * modernise `HydrogenBondAnalysis` to use `AtomGroups` as objects internally 
+    instead of selection strings
+
+Changes
+  * `guess_acceptors`, `guess_hydrogens` and `guess_donators` no longer return
+    selection strings (return `AtomGroups`instead)
+  * `_prepare` and `_single_frame` no longer pass selection strings to other
+    class methods (pass `AtomGroups` instead)
+
+Deprecations
+  * Selection string return of `guess_acceptors`, `guess_hydrogens` and 
+    `guess_donators` has been deprecated in favour of return of `AtomGroups`
+    and will removed in removed in version 3.0.0 (PR #4533)
+
 ??/??/?? IAlibay, HeetVekariya, marinegor, lilyminium, RMeli,
          ljwoods2, aditya292002, pstaerk, PicoCentauri, BFedder,
          tyler.je.reddy, SampurnaM, leonwehrhan, kainszs, orionarcher

package/MDAnalysis/analysis/hydrogenbonds/hbond_analysis.py

@@ -108,6 +108,10 @@
 
 Alternatively, :attr:`hydrogens_sel` and :attr:`acceptors_sel` may be generated via the :attr:`guess_hydrogens` and
 :attr:`guess_acceptors`. This selection strings may then be modified prior to calling :attr:`run`, or a subset of
+the universe may be used to guess the atoms. For example, find hydrogens and acceptors belonging to a protein::
+
+Alternatively, :attr:`hydrogens_ag` and :attr:`acceptors_ag` may be generated via the :attr:`guess_hydrogens` and
+:attr:`guess_acceptors`. This selection strings may then be modified prior to calling :attr:`run`, or a subset of
 the universe may be used to guess the atoms. For example, find hydrogens and acceptors belonging to a protein::
 
   import MDAnalysis
@@ -406,9 +410,9 @@ def guess_hydrogens(self,
 
         Returns
         -------
-        potential_hydrogens: str
-            String containing the :attr:`resname` and :attr:`name` of all 
-            hydrogen atoms potentially capable of forming hydrogen bonds.
+        potential_hydrogens: AtomGroup
+            AtomGroup containing all of the hydrogen atoms potentially
+            capable of forming hydrogen bonds.
 
         Notes
         -----
@@ -424,30 +428,24 @@ def guess_hydrogens(self,
         If :attr:`hydrogens_sel` is `None`, this function is called to guess 
         the selection.
 
-        Alternatively, this function may be used to quickly generate a 
-        :class:`str` of potential hydrogen atoms involved in hydrogen bonding.
-        This str may then be modified before being used to set the attribute
-        :attr:`hydrogens_sel`.
-
 
         .. versionchanged:: 2.4.0
             Added ability to use atom types
+        .. versionchanged:: 3.0.0
+            Returns AtomGroup instead of selection string
 
         """
 
         if min_mass > max_mass:
             raise ValueError("min_mass is higher than (or equal to) max_mass")
 
         ag = self.u.select_atoms(select)
-        hydrogens_ag = ag[
-            np.logical_and.reduce((
-                ag.masses < max_mass,
-                ag.charges > min_charge,
-                ag.masses > min_mass,
-            ))
-        ]
+        hydrogens_ag = ag.select_atoms(f"prop charge > {min_charge} and prop \
+                                       mass > {min_mass} and prop mass < \
+                                       {max_mass}", 
+                                       updating=self.update_selections)
 
-        return self._group_categories(hydrogens_ag)
+        return hydrogens_ag
 
     def guess_donors(self, select='all', max_charge=-0.5):
         """Guesses which atoms could be considered donors in the analysis. Only
@@ -465,9 +463,9 @@ def guess_donors(self, select='all', max_charge=-0.5):
 
         Returns
         -------
-        potential_donors: str
-            String containing the :attr:`resname` and :attr:`name` of all atoms 
-            that are potentially capable of forming hydrogen bonds.
+        potential_donors: AtomGroup
+            AtomGroup containing all of the atoms potentially capable of
+            forming hydrogen bonds.
 
         Notes
         -----
@@ -484,25 +482,23 @@ def guess_donors(self, select='all', max_charge=-0.5):
         have bonding information, this function is called to guess the 
         selection.
 
-        Alternatively, this function may be used to quickly generate a 
-        :class:`str` of potential donor atoms involved in hydrogen bonding. 
-        This :class:`str` may then be modified before being used to set the 
-        attribute :attr:`donors_sel`.
-
 
         .. versionchanged:: 2.4.0
             Added ability to use atom types
+        .. versionchanged:: 3.0.0
+            Returns AtomGroup instead of selection string
 
         """
 
         # We need to know `hydrogens_sel` before we can find donors
         # Use a new variable `hydrogens_sel` so that we do not set 
         # `self.hydrogens_sel` if it is currently `None`
         if self.hydrogens_sel is None:
-            hydrogens_sel = self.guess_hydrogens()
+            hydrogens_sel = self._group_categories(self.guess_hydrogens())
         else:
             hydrogens_sel = self.hydrogens_sel
-        hydrogens_ag = self.u.select_atoms(hydrogens_sel)
+        hydrogens_ag = self.u.select_atoms(hydrogens_sel, 
+                                           updating=self.update_selections)
 
         # We're using u._topology.bonds rather than u.bonds as it is a million
         # times faster to access. This is because u.bonds also calculates
@@ -521,9 +517,10 @@ def guess_donors(self, select='all', max_charge=-0.5):
                 )
             )
 
-        donors_ag = donors_ag[donors_ag.charges < max_charge]
+        donors_ag = donors_ag.select_atoms(f"prop charge < {max_charge}", 
+                                           updating=self.update_selections)
 
-        return self._group_categories(donors_ag)
+        return donors_ag
 
     def guess_acceptors(self, select='all', max_charge=-0.5):
         """Guesses which atoms could be considered acceptors in the analysis.
@@ -542,9 +539,9 @@ def guess_acceptors(self, select='all', max_charge=-0.5):
 
         Returns
         -------
-        potential_acceptors: str
-            String containing the :attr:`resname` and :attr:`name` of all atoms 
-            that potentially capable of forming hydrogen bonds.
+        potential_acceptors: AtomGroup
+            AtomGroup containing all of the atoms potentially capable of 
+            forming hydrogen bonds.
 
         Notes
         -----
@@ -559,21 +556,20 @@ def guess_acceptors(self, select='all', max_charge=-0.5):
         If :attr:`acceptors_sel` is `None`, this function is called to guess 
         the selection.
 
-        Alternatively, this function may be used to quickly generate a 
-        :class:`str` of potential acceptor atoms involved in hydrogen bonding. 
-        This :class:`str` may then be modified before being used to set the 
-        attribute :attr:`acceptors_sel`.
-
 
         .. versionchanged:: 2.4.0
             Added ability to use atom types
+        .. versionchanged:: 3.0.0
+            Returns AtomGroup instead of selection string
 
         """
 
         ag = self.u.select_atoms(select)
-        acceptors_ag = ag[ag.charges < max_charge]
+        #acceptors_ag = ag[ag.charges < max_charge]
+        acceptors_ag = ag.select_atoms(f"prop charge < {max_charge}", 
+                                       updating=self.update_selections)
 
-        return self._group_categories(acceptors_ag)
+        return acceptors_ag
 
     @staticmethod
     def _group_categories(group):
@@ -632,15 +628,21 @@ def _get_dh_pairs(self):
                                   'Please either: load a topology file with bond information; use the guess_bonds() '
                                   'topology guesser; or set HydrogenBondAnalysis.donors_sel so that a distance cutoff '
                                   'can be used.')
-
-            hydrogens = self.u.select_atoms(self.hydrogens_sel)
+            if self.hydrogens_sel is None:
+                hydrogens = self.guess_hydrogens()
+            else:
+                hydrogens = self.u.select_atoms(self.hydrogens_sel, 
+                                                updating=self.update_selections)
             donors = sum(h.bonded_atoms[0] for h in hydrogens) if hydrogens \
                 else AtomGroup([], self.u)
 
         # Otherwise, use d_h_cutoff as a cutoff distance
         else:
-
-            hydrogens = self.u.select_atoms(self.hydrogens_sel)
+            if self.hydrogens_sel is None:
+                hydrogens = self.guess_hydrogens()
+            else:
+                hydrogens = self.u.select_atoms(self.hydrogens_sel, 
+                                                updating=self.update_selections)
             donors = self.u.select_atoms(self.donors_sel)
             donors_indices, hydrogen_indices = capped_distance(
                 donors.positions,
@@ -699,27 +701,18 @@ def _filter_atoms(self, donors, acceptors):
     def _prepare(self):
         self.results.hbonds = [[], [], [], [], [], []]
 
-        # Set atom selections if they have not been provided
-        if self.acceptors_sel is None:
-            self.acceptors_sel = self.guess_acceptors()
-        if self.hydrogens_sel is None:
-            self.hydrogens_sel = self.guess_hydrogens()
-
         # Select atom groups
-        self._acceptors = self.u.select_atoms(self.acceptors_sel,
-                                              updating=self.update_selections)
+        if self.acceptors_sel is None:
+            self._acceptors = self.guess_acceptors()
+        else:
+            self._acceptors = self.u.select_atoms(self.acceptors_sel,
+                                                updating=self.update_selections)
         self._donors, self._hydrogens = self._get_dh_pairs()
 
     def _single_frame(self):
 
         box = self._ts.dimensions
 
-        # Update donor-hydrogen pairs if necessary
-        if self.update_selections:
-            self._donors, self._hydrogens = self._get_dh_pairs()
-
-        # find D and A within cutoff distance of one another
-        # min_cutoff = 1.0 as an atom cannot form a hydrogen bond with itself
         d_a_indices, d_a_distances = capped_distance(
             self._donors.positions,
             self._acceptors.positions,

testsuite/MDAnalysisTests/analysis/test_hydrogenbonds_analysis.py

@@ -227,7 +227,7 @@ def test_no_bond_donor_sel(self, universe):
         u.add_TopologyAttr('mass', [15.999, 1.008, 1.008] * n_residues)
         u.add_TopologyAttr('charge', [-1.04, 0.52, 0.52] * n_residues)
         h = HydrogenBondAnalysis(u, **kwargs)
-        donors = u.select_atoms(h.guess_donors())
+        donors = h.guess_donors()
 
     def test_first_hbond(self, hydrogen_bonds):
         assert len(hydrogen_bonds.results.hbonds) == 2
@@ -554,7 +554,8 @@ def test_guess_donors(self, h):
 
         ref_donors = "(resname TIP3 and name OH2)"
         donors = h.guess_donors(select='all', max_charge=-0.5)
-        assert donors == ref_donors
+        donors_sel = h._group_categories(donors)
+        assert donors_sel == ref_donors
 
 
 class TestHydrogenBondAnalysisTIP3P_GuessHydrogens_NoTopology(object):
@@ -586,7 +587,8 @@ def test_guess_hydrogens(self, h):
 
         ref_hydrogens = "(resname TIP3 and name H1) or (resname TIP3 and name H2)"
         hydrogens = h.guess_hydrogens(select='all')
-        assert hydrogens == ref_hydrogens
+        hydrogens_sel = h._group_categories(hydrogens)
+        assert hydrogens_sel == ref_hydrogens
 
     pytest.mark.parametrize(
         "min_mass, max_mass, min_charge",
@@ -599,7 +601,7 @@ def test_guess_hydrogens(self, h):
     def test_guess_hydrogens_empty_selection(self, h):
 
         hydrogens = h.guess_hydrogens(select='all', min_charge=1.0)
-        assert hydrogens == ""
+        assert len(hydrogens) == 0
 
     def test_guess_hydrogens_min_max_mass(self, h):