Fix the case where distance_type = 'heavy' in water bridge analysis (#…

…4066)

* fix #4040 and #4136 
* ensure that for distance_type == 'heavy' all possible hydrogens are taken into account when analyzing H-bonds;
  previously, only one connected H was considered and that could lead to incorrect results and IndexErrors
* ensure that WaterBridgeAnalysis ONLY accepts distance_type 'heavy' or 'hydrogen' (anything else now
  raises ValueError)
* add tests
* update CHANGELOG
* Long explanation (see also PR #4066)

  In water bridge analysis, in the distance_type = 'hydrogen' mode, I get the pair index (hydrogen bond hydrogen atom   and acceptor) and the pair distance from the distance search.
  Then for each hydrogen atom bonded to the donor heavy atom, I check the angle between the hydrogen bond donor heavy atom, the hydrogen atom and the acceptor and map the index to the pair distance. This works because the  distance is computed based on the hydrogen atom and we map the hydrogen atom index back to the distance.

  In the distance_type = 'heavy' mode, In the original code, I get the pair index (hydrogen bond donor heavy atom and acceptor) and the pair distance from the distance search. Then for each hydrogen atom bonded to the donor heavy atom, I check the angle between the hydrogen bond donor heavy atom, the hydrogen atom and the acceptor. In this case, I cannot map back to the pair distance, as the distance search is done on the heavy atom and we are mapping the index of the hydrogen atom back to the distance.

 This PR fixes the issue.

Co-authored-by: Irfan Alibay <IAlibay@users.noreply.github.com>
Co-authored-by: Oliver Beckstein <orbeckst@gmail.com>

package/CHANGELOG

@@ -17,11 +17,14 @@ The rules for this file:
 ??/??/?? IAlibay, pgbarletta, mglagolev, hmacdope, manuel.nuno.melo, chrispfae, 
          ooprathamm, MeetB7, BFedder, v-parmar, MoSchaeffler, jbarnoud, jandom,
          xhgchen, jaclark5, DrDomenicoMarson, AHMED-salah00, schlaicha,
-         SophiaRuan, g2707, p-j-smith, tylerjereddy
+         SophiaRuan, g2707, p-j-smith, tylerjereddy, xiki-tempula
 
  * 2.5.0
 
 Fixes
+  * Fix the `heavy` distance_type for water bridge analysis where distance
+    is not correctly assigned when more than one hydrogen is bonded to a
+    heavy atom (Issue #4040, PR #4066).
   * PDB topology parser no longer fails when encountering unknown formal
     charges and instead simply does not populate attribute (Issue #4027)
   * Added support for Cython 3.0.0b2 (PR #4129)

package/MDAnalysis/analysis/hydrogenbonds/wbridge_analysis.py

@@ -722,7 +722,6 @@ def analysis(current, output, u, **kwargs):
 from MDAnalysis.lib.NeighborSearch import AtomNeighborSearch
 from MDAnalysis.lib.distances import capped_distance, calc_angles
 from MDAnalysis import NoDataError, MissingDataWarning, SelectionError
-from MDAnalysis.lib import distances
 
 logger = logging.getLogger('MDAnalysis.analysis.WaterBridgeAnalysis')
 
@@ -769,7 +768,7 @@ class WaterBridgeAnalysis(AnalysisBase):
     #: N, O, P, and S. Any other heavy atoms are assumed to have hydrogens
     #: covalently bound at a maximum distance of 1.5 Å.
     r_cov = defaultdict(lambda: 1.5,  # default value
-                        N=1.31, O=1.31, P=1.58, S=1.55)
+                        N=1.31, O=1.31, P=1.58, S=1.55)  # noqa: E741
 
     def __init__(self, universe, selection1='protein',
                  selection2='not resname SOL', water_selection='resname SOL',
@@ -866,8 +865,9 @@ def __init__(self, universe, selection1='protein',
             :attr:`~DEFAULT_ACCEPTORS` values.
             ["CHARMM27"]
         donors : sequence (optional)
-            Extra H donor atom types (in addition to those in
-            :attr:`~DEFAULT_DONORS`), must be a sequence.
+            Extra H donor atom types (in addition to those in :attr:`~DEFAULT_DONORS`).
+            This shall be the name of the heavy atom that is bonded to the hydrogen.
+            For example, the oxygen ('O') in the hydroxyl group. Must be a sequence.
         acceptors : sequence (optional)
             Extra H acceptor atom types (in addition to those in
             :attr:`~DEFAULT_ACCEPTORS`), must be a
@@ -925,6 +925,8 @@ def __init__(self, universe, selection1='protein',
         self.selection1 = selection1
         self.selection2 = selection2
         self.selection1_type = selection1_type
+        if "selection2_type" in kwargs:
+            raise ValueError("`selection2_type` is not a keyword argument.")
 
         # if the selection 1 and selection 2 are the same
         if selection1 == selection2:
@@ -933,7 +935,9 @@ def __init__(self, universe, selection1='protein',
         self.update_selection = update_selection
         self.filter_first = filter_first
         self.distance = distance
-        self.distance_type = distance_type  # note: everything except 'heavy'
+        if distance_type not in {"hydrogen", "heavy"}:
+            raise ValueError(f"Only 'hydrogen' and 'heavy' are allowed for option `distance_type' ({distance_type}).")
+        self.distance_type = distance_type
         # will give the default behavior
         self.angle = angle
         self.pbc = pbc and all(self.u.dimensions[:3])
@@ -1209,19 +1213,25 @@ def _donor2acceptor(self, donors, h_donors, acceptor):
                                            max_cutoff=self.distance,
                                            box=self.box,
                                            return_distances=True)
-        if self.distance_type != 'heavy':
+        if self.distance_type == 'hydrogen':
+            tmp_distances = distances
             tmp_donors = [h_donors[donors_idx[idx]] for idx in pairs[:, 0]]
             tmp_hydrogens = [donors_idx[idx] for idx in pairs[:, 0]]
             tmp_acceptors = [acceptor[idx] for idx in pairs[:, 1]]
         else:
+            # To make sure that for the same index i, the donor (tmp_donors[i]),
+            # hydrogen (tmp_hydrogens[i]), acceptor (tmp_acceptors[i]) matches the
+            # distance (tmp_distances[i]).
             tmp_donors = []
             tmp_hydrogens = []
             tmp_acceptors = []
-            for idx in range(len(pairs[:, 0])):
+            tmp_distances = []
+            for idx, distance in enumerate(distances):
                 for h in donors[donors_idx[pairs[idx, 0]]]:
                     tmp_donors.append(donors_idx[pairs[idx, 0]])
                     tmp_hydrogens.append(h)
                     tmp_acceptors.append(acceptor[pairs[idx, 1]])
+                    tmp_distances.append(distance)
 
         angles = np.rad2deg(
             calc_angles(
@@ -1238,7 +1248,7 @@ def _donor2acceptor(self, donors, h_donors, acceptor):
             a = tmp_acceptors[index]
             result.append((h, d, a, self._expand_index(h),
                            self._expand_index(a),
-                           distances[index], angles[index]))
+                           tmp_distances[index], angles[index]))
         return result
 
     def _single_frame(self):