Issue #2395 AtomGroup.guess_bonds

package/AUTHORS

@@ -209,6 +209,7 @@ Chronological list of authors
   - Vishal Parmar
   - Moritz Schaeffler
   - Xu Hong Chen
+  - Ahmed Salah Ghoneim
 
 External code
 -------------

package/CHANGELOG

@@ -16,10 +16,12 @@ The rules for this file:
 
 ??/??/?? IAlibay, pgbarletta, mglagolev, hmacdope, manuel.nuno.melo, chrispfae, 
          ooprathamm, MeetB7, BFedder, v-parmar, MoSchaeffler, jbarnoud, jandom,
-         xhgchen, jaclark5
+         xhgchen, jaclark5, AHMED-salah00
  * 2.5.0
 
 Fixes
+  * Fix the misleading 'AtomGroup.guess_bonds()' docs and passed missing
+    arguments (PR #4059)
   * Allows shape_parameter and asphericity to yield per residue quantities
     (Issue #3002, PR #3905)
   * Add tests for "No path data" exception raise in test_psa.py (Issue #4036)

package/MDAnalysis/core/groups.py

@@ -3276,24 +3276,38 @@ def split(self, level):
         return [self[levelindices == index] for index in
                 unique_int_1d(levelindices)]
 
-    def guess_bonds(self, vdwradii=None):
-        """Guess bonds that exist within this :class:`AtomGroup` and add them to
-        the underlying :attr:`~AtomGroup.universe`.
+    def guess_bonds(self, vdwradii=None, fudge_factor=0.55, lower_bound=0.1):
+        """Guess bonds, angles, and dihedrals between the atoms in this
+        :class:`AtomGroup` and add them to the underlying
+        :attr:`~AtomGroup.universe`.
 
         Parameters
         ----------
         vdwradii : dict, optional
             Dict relating atom types: vdw radii
 
+        fudge_factor : float, optional
+            The factor by which atoms must overlap each other to be considered
+            a bond.  Larger values will increase the number of bonds found. [0.55]
+        lower_bound : float, optional
+            The minimum bond length. All bonds found shorter than this length
+            will be ignored. This is useful for parsing PDB with altloc records
+            where atoms with altloc A and B may be very close together and
+            there should be no chemical bond between them. [0.1]
 
         See Also
         --------
         :func:`MDAnalysis.topology.guessers.guess_bonds`
+        :func:`MDAnalysis.topology.guessers.guess_angles`
+        :func:`MDAnalysis.topology.guessers.guess_dihedrals`
 
 
         .. versionadded:: 0.10.0
         .. versionchanged:: 0.20.2
            Now applies periodic boundary conditions when guessing bonds.
+        .. versionchanged:: 2.5.0
+           Corrected misleading docs, and now allows passing of `fudge_factor`
+           and `lower_bound` arguments.
         """
         from ..topology.core import guess_bonds, guess_angles, guess_dihedrals
         from .topologyattrs import Bonds, Angles, Dihedrals
@@ -3308,9 +3322,15 @@ def get_TopAttr(u, name, cls):
                 return attr
 
         # indices of bonds
-        b = guess_bonds(self.atoms, self.atoms.positions,
-                        vdwradii=vdwradii, box=self.dimensions)
-        bondattr = get_TopAttr(self.universe, 'bonds', Bonds)
+        b = guess_bonds(
+            self.atoms,
+            self.atoms.positions,
+            vdwradii=vdwradii,
+            box=self.dimensions,
+            fudge_factor=fudge_factor,
+            lower_bound=lower_bound,
+        )
+        bondattr = get_TopAttr(self.universe, "bonds", Bonds)
         bondattr._add_bonds(b, guessed=True)
 
         a = guess_angles(self.bonds)

package/MDAnalysis/core/universe.py

@@ -238,6 +238,11 @@ class Universe(object):
     vdwradii: dict, ``None``, default ``None``
         For use with *guess_bonds*. Supply a dict giving a vdwradii for each
         atom type which are used in guessing bonds.
+    fudge_factor: float, default [0.55]
+        For use with *guess_bonds*. Supply the factor by which atoms must
+        overlap each other to be considered a bond.
+    lower_bound: float, default [0.1]
+        For use with *guess_bonds*. Supply the minimum bond length.
     transformations: function or list, ``None``, default ``None``
         Provide a list of transformations that you wish to apply to the
         trajectory upon reading. Transformations can be found in
@@ -312,11 +317,14 @@ class Universe(object):
     .. versionchanged:: 2.0.0
         Universe now can be (un)pickled.
         ``topology`` and ``trajectory`` are reserved upon unpickle.
-
+    .. versionchanged:: 2.5.0
+        Added fudge_factor and lower_bound parameters for use with
+        *guess_bonds*.
     """
     def __init__(self, topology=None, *coordinates, all_coordinates=False,
                  format=None, topology_format=None, transformations=None,
-                 guess_bonds=False, vdwradii=None, in_memory=False,
+                 guess_bonds=False, vdwradii=None, fudge_factor=0.55,
+                 lower_bound=0.1, in_memory=False,
                  in_memory_step=1, **kwargs):
 
         self._trajectory = None  # managed attribute holding Reader
@@ -330,6 +338,8 @@ def __init__(self, topology=None, *coordinates, all_coordinates=False,
             'transformations': transformations,
             'guess_bonds': guess_bonds,
             'vdwradii': vdwradii,
+            'fudge_factor': fudge_factor,
+            'lower_bound': lower_bound,
             'in_memory': in_memory,
             'in_memory_step': in_memory_step,
             'format': format,
@@ -371,7 +381,8 @@ def __init__(self, topology=None, *coordinates, all_coordinates=False,
             self._trajectory.add_transformations(*transformations)
 
         if guess_bonds:
-            self.atoms.guess_bonds(vdwradii=vdwradii)
+            self.atoms.guess_bonds(vdwradii=vdwradii, fudge_factor=fudge_factor,
+                                   lower_bound=lower_bound)
 
     def copy(self):
         """Return an independent copy of this Universe"""

testsuite/MDAnalysisTests/core/test_universe.py

@@ -398,7 +398,7 @@ class TestGuessBonds(object):
      - fail properly if not
      - work again if vdwradii are passed.
     """
-    @pytest.fixture()
+    @pytest.fixture(scope='module')
     def vdw(self):
         return {'A': 1.4, 'B': 0.5}
 
@@ -424,7 +424,7 @@ def test_universe_guess_bonds(self):
     def test_universe_guess_bonds_no_vdwradii(self):
         """Make a Universe that has atoms with unknown vdwradii."""
         with pytest.raises(ValueError):
-            mda.Universe(two_water_gro_nonames, guess_bonds = True)
+            mda.Universe(two_water_gro_nonames, guess_bonds=True)
 
     def test_universe_guess_bonds_with_vdwradii(self, vdw):
         """Unknown atom types, but with vdw radii here to save the day"""
@@ -441,6 +441,17 @@ def test_universe_guess_bonds_off(self):
             assert not hasattr(u, attr)
         assert not u.kwargs['guess_bonds']
 
+    def test_universe_guess_bonds_arguments(self):
+        """Test if 'fudge_factor', and 'lower_bound' parameters
+        are being passed correctly.
+        """
+        u = mda.Universe(two_water_gro, guess_bonds=True)
+
+        self._check_universe(u)
+        assert u.kwargs["guess_bonds"]
+        assert u.kwargs["fudge_factor"]
+        assert u.kwargs["lower_bound"]
+
     def _check_atomgroup(self, ag, u):
         """Verify that the AtomGroup made bonds correctly,
         and that the Universe got all this info
@@ -458,13 +469,20 @@ def _check_atomgroup(self, ag, u):
         assert_equal(len(u.atoms[4].bonds), 0)
         assert_equal(len(u.atoms[5].bonds), 0)
 
-    def test_atomgroup_guess_bonds(self):
+    @pytest.mark.parametrize(
+        'ff, lb, nbonds',
+        [
+            (0.55, 0.1, 2), (0.9, 1.6, 1),
+            (0.5, 0.2, 2), (0.1, 0.1, 0)
+        ]
+    )
+    def test_atomgroup_guess_bonds(self, ff, lb, nbonds):
         """Test an atomgroup doing guess bonds"""
         u = mda.Universe(two_water_gro)
 
         ag = u.atoms[:3]
-        ag.guess_bonds()
-        self._check_atomgroup(ag, u)
+        ag.guess_bonds(fudge_factor=ff, lower_bound=lb)
+        assert len(ag.bonds) == nbonds
 
     def test_atomgroup_guess_bonds_no_vdwradii(self):
         u = mda.Universe(two_water_gro_nonames)