diff --git a/package/MDAnalysis/lib/distances.py b/package/MDAnalysis/lib/distances.py index c0d994862c2..b1509e72df0 100644 --- a/package/MDAnalysis/lib/distances.py +++ b/package/MDAnalysis/lib/distances.py @@ -336,7 +336,7 @@ def capped_distance(reference, configuration, max_cutoff, min_cutoff=None, An automatic guessing of the optimal method to calculate the distances is included in the function. An optional keyword for the method is also provided. Users can enforce a particular method with this functionality. - Currently brute force, grid search, and periodic KDtree methods are + Currently brute force and periodic KDtree methods are implemented. Parameters @@ -354,7 +354,7 @@ def capped_distance(reference, configuration, max_cutoff, min_cutoff=None, triclinic and must be provided in the same format as returned by :attr:`MDAnalysis.coordinates.base.Timestep.dimensions`:\n ``[lx, ly, lz, alpha, beta, gamma]``. - method : {'bruteforce', 'nsgrid', 'pkdtree'}, optional + method : {'bruteforce', 'pkdtree'}, optional Keyword to override the automatic guessing of the employed search method. return_distances : bool, optional @@ -387,14 +387,18 @@ def capped_distance(reference, configuration, max_cutoff, min_cutoff=None, Note ----- - Currently supports brute force, grid-based, and periodic KDtree search + Currently supports brute force, and periodic KDtree search methods. See Also -------- distance_array MDAnalysis.lib.pkdtree.PeriodicKDTree.search - MDAnalysis.lib.nsgrid.FastNS.search + + + .. versionchanged:: 1.0.1 + nsgrid was temporarily removed and replaced with pkdtree due to issues + surround its reliability (Issues #2919, #2229, #2345, #2670, #2930) """ if box is not None: box = np.asarray(box, dtype=np.float32) @@ -430,7 +434,7 @@ def _determine_method(reference, configuration, max_cutoff, min_cutoff=None, triclinic and must be provided in the same format as returned by :attr:`MDAnalysis.coordinates.base.Timestep.dimensions`:\n ``[lx, ly, lz, alpha, beta, gamma]``. - method : {'bruteforce', 'nsgrid', 'pkdtree'}, optional + method : {'bruteforce', 'pkdtree'}, optional Keyword to override the automatic guessing of the employed search method. @@ -438,10 +442,15 @@ def _determine_method(reference, configuration, max_cutoff, min_cutoff=None, ------- function : callable The function implementing the guessed (or deliberatly chosen) method. + + + .. versionchanged:: 1.0.1 + nsgrid was temporarily removed and replaced with pkdtree due to issues + surround its reliability (Issues #2919, #2229, #2345, #2670, #2930) """ + # TODO: add 'nsgrid': _nsgrid_capped back once fixed methods = {'bruteforce': _bruteforce_capped, - 'pkdtree': _pkdtree_capped, - 'nsgrid': _nsgrid_capped} + 'pkdtree': _pkdtree_capped} if method is not None: return methods[method.lower()] @@ -451,7 +460,8 @@ def _determine_method(reference, configuration, max_cutoff, min_cutoff=None, elif len(reference) * len(configuration) >= 1e8: # CAUTION : for large datasets, shouldnt go into 'bruteforce' # in any case. Arbitrary number, but can be characterized - return methods['nsgrid'] + # Temporarily replace nsgrid with pkdtree Issue #2930 + return methods['pkdtree'] else: if box is None: min_dim = np.array([reference.min(axis=0), @@ -467,7 +477,8 @@ def _determine_method(reference, configuration, max_cutoff, min_cutoff=None, if np.any(max_cutoff > 0.3*size): return methods['bruteforce'] else: - return methods['nsgrid'] + # Temporarily replace nsgrid with pkdtree Issue #2930 + return methods['pkdtree'] @check_coords('reference', 'configuration', enforce_copy=False, @@ -741,7 +752,7 @@ def self_capped_distance(reference, max_cutoff, min_cutoff=None, box=None, An automatic guessing of the optimal method to calculate the distances is included in the function. An optional keyword for the method is also provided. Users can enforce a particular method with this functionality. - Currently brute force, grid search, and periodic KDtree methods are + Currently brute force, and periodic KDtree methods are implemented. Parameters @@ -757,7 +768,7 @@ def self_capped_distance(reference, max_cutoff, min_cutoff=None, box=None, triclinic and must be provided in the same format as returned by :attr:`MDAnalysis.coordinates.base.Timestep.dimensions`:\n ``[lx, ly, lz, alpha, beta, gamma]``. - method : {'bruteforce', 'nsgrid', 'pkdtree'}, optional + method : {'bruteforce', 'pkdtree'}, optional Keyword to override the automatic guessing of the employed search method. return_distances : bool, optional @@ -789,17 +800,20 @@ def self_capped_distance(reference, max_cutoff, min_cutoff=None, box=None, Note ----- - Currently supports brute force, grid-based, and periodic KDtree search + Currently supports brute force, and periodic KDtree search methods. See Also -------- self_distance_array MDAnalysis.lib.pkdtree.PeriodicKDTree.search - MDAnalysis.lib.nsgrid.FastNS.self_search + .. versionchanged:: 0.20.0 Added `return_distances` keyword. + .. versionchanged:: 1.0.1 + nsgrid was temporarily removed and replaced with pkdtree due to issues + surround its reliability (Issues #2919, #2229, #2345, #2670, #2930) """ if box is not None: box = np.asarray(box, dtype=np.float32) @@ -833,7 +847,7 @@ def _determine_method_self(reference, max_cutoff, min_cutoff=None, box=None, triclinic and must be provided in the same format as returned by :attr:`MDAnalysis.coordinates.base.Timestep.dimensions`:\n ``[lx, ly, lz, alpha, beta, gamma]``. - method : {'bruteforce', 'nsgrid', 'pkdtree'}, optional + method : {'bruteforce', 'pkdtree'}, optional Keyword to override the automatic guessing of the employed search method. @@ -841,10 +855,15 @@ def _determine_method_self(reference, max_cutoff, min_cutoff=None, box=None, ------- function : callable The function implementing the guessed (or deliberatly chosen) method. + + + .. versionchanged:: 1.0.1 + nsgrid was temporarily removed and replaced with pkdtree due to issues + surround its reliability (Issues #2919, #2229, #2345, #2670, #2930) """ + # TODO: add 'nsgrid': _nsgrid_capped back once fixed methods = {'bruteforce': _bruteforce_capped_self, - 'pkdtree': _pkdtree_capped_self, - 'nsgrid': _nsgrid_capped_self} + 'pkdtree': _pkdtree_capped_self} if method is not None: return methods[method.lower()] @@ -865,7 +884,8 @@ def _determine_method_self(reference, max_cutoff, min_cutoff=None, box=None, if max_cutoff < 0.03*size.min(): return methods['pkdtree'] else: - return methods['nsgrid'] + # Replaced nsgrid with pkdtree temporarily #2930 + return methods['pkdtree'] @check_coords('reference', enforce_copy=False, reduce_result_if_single=False) diff --git a/package/MDAnalysis/lib/nsgrid.pyx b/package/MDAnalysis/lib/nsgrid.pyx index eadd8edf6ef..047003e4e64 100644 --- a/package/MDAnalysis/lib/nsgrid.pyx +++ b/package/MDAnalysis/lib/nsgrid.pyx @@ -70,6 +70,15 @@ not reflect in the results. .. versionadded:: 0.19.0 +Important Note +-------------- + +Several issues have been raised about the nsgrid code (#2919, #2229, #2345, +#2670, #2930). In summary, this code currently yield unreliable/inaccurate +results. Until fixed, it should not be used. We have left this module in +place as we prepare an fix for these errors. In the meantime, use of this +library is at your own perils. + Classes ------- """ diff --git a/testsuite/MDAnalysisTests/lib/test_distances.py b/testsuite/MDAnalysisTests/lib/test_distances.py index cb02dbd8cf2..c691b7c5e8c 100644 --- a/testsuite/MDAnalysisTests/lib/test_distances.py +++ b/testsuite/MDAnalysisTests/lib/test_distances.py @@ -96,7 +96,7 @@ def test_capped_distance_noresults(): np.array([[0.1, 0.1, 0.1], [0.2, 0.1, 0.1]], dtype=np.float32)) -method_1 = ('bruteforce', 'pkdtree', 'nsgrid') +method_1 = ('bruteforce', 'pkdtree') # add nsgrid back min_cutoff_1 = (None, 0.1) @@ -220,7 +220,7 @@ def test_self_capped_distance(npoints, box, method, min_cutoff, ret_dist): [(1, 0.02, '_bruteforce_capped_self'), (1, 0.2, '_bruteforce_capped_self'), (600, 0.02, '_pkdtree_capped_self'), - (600, 0.2, '_nsgrid_capped_self')]) + (600, 0.2, '_pkdtree_capped_self')]) # tmp switch to pkdtree def test_method_selfselection(box, npoints, cutoff, meth): np.random.seed(90003) points = (np.random.uniform(low=0, high=1.0, @@ -235,9 +235,9 @@ def test_method_selfselection(box, npoints, cutoff, meth): @pytest.mark.parametrize('npoints,cutoff,meth', [(1, 0.02, '_bruteforce_capped'), (1, 0.2, '_bruteforce_capped'), - (200, 0.02, '_nsgrid_capped'), + (200, 0.02, '_pkdtree_capped'), # tmp switch to pkdtree (200, 0.35, '_bruteforce_capped'), - (10000, 0.35, '_nsgrid_capped')]) + (10000, 0.35, '_pkdtree_capped')]) # tmp switch to pkdtree def test_method_selection(box, npoints, cutoff, meth): np.random.seed(90003) points = (np.random.uniform(low=0, high=1.0, @@ -991,7 +991,7 @@ def test_input_unchanged_self_distance_array(self, coords, box, backend): assert_equal(crd, ref) @pytest.mark.parametrize('box', boxes) - @pytest.mark.parametrize('met', ["bruteforce", "pkdtree", "nsgrid", None]) + @pytest.mark.parametrize('met', ["bruteforce", "pkdtree", None]) # add nsgrid back def test_input_unchanged_capped_distance(self, coords, box, met): crds = coords[:2] refs = [crd.copy() for crd in crds] @@ -1000,7 +1000,7 @@ def test_input_unchanged_capped_distance(self, coords, box, met): assert_equal(crds, refs) @pytest.mark.parametrize('box', boxes) - @pytest.mark.parametrize('met', ["bruteforce", "pkdtree", "nsgrid", None]) + @pytest.mark.parametrize('met', ["bruteforce", "pkdtree", None]) # add nsgrid back def test_input_unchanged_self_capped_distance(self, coords, box, met): crd = coords[0] ref = crd.copy() @@ -1101,7 +1101,7 @@ def test_empty_input_self_distance_array(self, empty_coord, box, backend): @pytest.mark.parametrize('box', boxes) @pytest.mark.parametrize('min_cut', [min_cut, None]) @pytest.mark.parametrize('ret_dist', [False, True]) - @pytest.mark.parametrize('met', ["bruteforce", "pkdtree", "nsgrid", None]) + @pytest.mark.parametrize('met', ["bruteforce", "pkdtree", None]) # add nsgrid back def test_empty_input_capped_distance(self, empty_coord, min_cut, box, met, ret_dist): res = distances.capped_distance(empty_coord, empty_coord, @@ -1117,7 +1117,7 @@ def test_empty_input_capped_distance(self, empty_coord, min_cut, box, met, @pytest.mark.parametrize('box', boxes) @pytest.mark.parametrize('min_cut', [min_cut, None]) @pytest.mark.parametrize('ret_dist', [False, True]) - @pytest.mark.parametrize('met', ["bruteforce", "pkdtree", "nsgrid", None]) + @pytest.mark.parametrize('met', ["bruteforce", "pkdtree", None]) # add nsgrid back def test_empty_input_self_capped_distance(self, empty_coord, min_cut, box, met, ret_dist): res = distances.self_capped_distance(empty_coord, @@ -1234,7 +1234,7 @@ def test_output_type_self_distance_array(self, incoords, box, backend): @pytest.mark.parametrize('min_cut', [min_cut, None]) @pytest.mark.parametrize('ret_dist', [False, True]) @pytest.mark.parametrize('incoords', list(comb(coords, 2))) - @pytest.mark.parametrize('met', ["bruteforce", "pkdtree", "nsgrid", None]) + @pytest.mark.parametrize('met', ["bruteforce", "pkdtree", None]) # add nsgrid back def test_output_type_capped_distance(self, incoords, min_cut, box, met, ret_dist): res = distances.capped_distance(*incoords, max_cutoff=self.max_cut, @@ -1257,7 +1257,7 @@ def test_output_type_capped_distance(self, incoords, min_cut, box, met, @pytest.mark.parametrize('min_cut', [min_cut, None]) @pytest.mark.parametrize('ret_dist', [False, True]) @pytest.mark.parametrize('incoords', coords) - @pytest.mark.parametrize('met', ["bruteforce", "pkdtree", "nsgrid", None]) + @pytest.mark.parametrize('met', ["bruteforce", "pkdtree", None]) # add nsgrid back def test_output_type_self_capped_distance(self, incoords, min_cut, box, met, ret_dist): res = distances.self_capped_distance(incoords,