From 5a59a26f5a4344ece34f422dbdf182a3517f89d8 Mon Sep 17 00:00:00 2001 From: Mike German Date: Wed, 29 Jul 2026 23:47:36 -0400 Subject: [PATCH 1/3] fix three bugs in dynamics/analysis.py: dropped hinges, IndexError, wrong collectivity norm getGlobalHinges was silently discarding hinges from every chain but the last: the accumulation block was indented one level too high, outside the per-chain loop, so it only ever ran once using the leftover h/l/fst from the final chain. Moved it inside the loop. Also fixed an off-by-one in the same function: chains stores inclusive (fst, lst) pairs but the vecs slice used the exclusive vecs[fst:lst], dropping the last residue of every chain from hinge detection. Now vecs[fst:lst+1, i]. getHinges raised IndexError (regs[0] on an empty list) whenever an eigenvector segment had no sign change at all. Added a guard that returns an empty hinge list in that case, matching the function's normal return type. calcCollectivity normalized u2in by dividing by sqrt(sum) instead of sum when masses were supplied, which doesn't produce a valid probability distribution per Bruschweiler 1995 eq. 5 (the exponent no longer sums the correct entropy term). Only the masses path was affected; the default unit-mass path is unchanged since sum and sqrt(sum) both equal 1 there. Added prody/tests/dynamics/test_analysis_hinge_bugs.py with synthetic, offline, deterministic repros for all three (multi-chain AtomGroup + monkeypatched _getModeVecs for the hinge bugs, hand-computed Bruschweiler values for collectivity). All 6 new tests fail on the prior code and pass after the fix; existing dynamics test modules show no regressions. Co-Authored-By: Claude --- prody/dynamics/analysis.py | 17 +-- .../dynamics/test_analysis_hinge_bugs.py | 117 ++++++++++++++++++ 2 files changed, 127 insertions(+), 7 deletions(-) create mode 100644 prody/tests/dynamics/test_analysis_hinge_bugs.py diff --git a/prody/dynamics/analysis.py b/prody/dynamics/analysis.py index 8c279edac..54fcff593 100644 --- a/prody/dynamics/analysis.py +++ b/prody/dynamics/analysis.py @@ -87,7 +87,7 @@ def log0(a): if len(masses) != n_atoms: raise ValueError('length of masses must be equal to number of atoms') u2in = u2in / masses - u2in = u2in * (1 / u2in.sum() ** 0.5) + u2in = u2in / u2in.sum() coll = np.exp(-(u2in * log0(u2in)).sum()) / n_atoms colls.append(coll) @@ -685,6 +685,9 @@ def getHinges(v, threshold=15, space=None): ### Merge overlapping or adjacent regions (separated by space value) ### + if not regs: + return [] + regs = sorted(regs, key=lambda x: x[0]) merged = [regs[0]] s = 1 + space if space is not None else 0 @@ -804,12 +807,12 @@ def getGlobalHinges(gnm, n_modes=None, threshold=15, space=None, atoms=None, min for fst, lst in chains: l = lst + 1 - fst - h = getHinges(vecs[fst:lst, i], threshold, space) - - if trim is not False: - hinges.extend(x + fst for x in h if trim <= x < l - trim) - else: - hinges.extend(x + fst for x in h) + h = getHinges(vecs[fst:lst + 1, i], threshold, space) + + if trim is not False: + hinges.extend(x + fst for x in h if trim <= x < l - trim) + else: + hinges.extend(x + fst for x in h) n_hinges.append(sorted(hinges)) diff --git a/prody/tests/dynamics/test_analysis_hinge_bugs.py b/prody/tests/dynamics/test_analysis_hinge_bugs.py new file mode 100644 index 000000000..6c87e04eb --- /dev/null +++ b/prody/tests/dynamics/test_analysis_hinge_bugs.py @@ -0,0 +1,117 @@ +"""Regression tests for three bugs found in prody.dynamics.analysis: + +1. getGlobalHinges silently drops hinges from every chain except the last + because the accumulation block is mis-indented outside the per-chain + loop, and also slices vecs with an exclusive upper bound (vecs[fst:lst]) + even though `chains` stores inclusive (fst, lst) pairs, dropping the + last atom of every chain from consideration. +2. getHinges raises IndexError when an eigenvector segment has no sign + change at all (regs ends up empty and `regs[0]` blows up). +3. calcCollectivity normalizes by dividing by sqrt(sum) instead of sum when + masses are supplied, which breaks the probability-distribution + requirement of the Bruschweiler 1995 (eq. 5) collectivity definition. +""" + +import unittest +from unittest.mock import patch + +import numpy as np +from prody import * +from prody.dynamics.analysis import getHinges, getGlobalHinges, calcCollectivity + + +def _make_two_chain_atoms(n1, n2): + """Tiny synthetic 2-chain AtomGroup (CA-only) for hinge tests.""" + n = n1 + n2 + ag = AtomGroup('synthetic') + ag.setCoords(np.zeros((n, 3))) + ag.setChids(['A'] * n1 + ['B'] * n2) + ag.setResnums(list(range(1, n1 + 1)) + list(range(1, n2 + 1))) + ag.setNames(['CA'] * n) + ag.setResnames(['ALA'] * n) + return ag + + +class TestGetHingesEmptyRegions(unittest.TestCase): + """Bug 2: getHinges must not crash when there is no sign change.""" + + def test_no_sign_change_returns_empty_list(self): + v = np.array([1.0, 1.0, 1.0, 1.0]) + # Should return [] (no hinges), not raise IndexError. + hinges = getHinges(v) + self.assertEqual(hinges, []) + + def test_normal_case_still_works(self): + # Sanity check the fix doesn't break the normal path. + v = np.array([-1, -1, -1, 1, 1, 1.0]) + self.assertEqual(getHinges(v), [2]) + + +class TestGetGlobalHingesChainAccumulation(unittest.TestCase): + """Bug 1a: hinges from all chains but the last are silently dropped.""" + + def test_all_chains_contribute_hinges(self): + ag = _make_two_chain_atoms(6, 5) + + # Chain A (global idx 0-5): crossing well inside -> local hinge 2 + v_a = np.array([-1, -1, -1, 1, 1, 1.0]) + # Chain B (global idx 6-10): crossing well inside -> local hinge 1 + v_b = np.array([1, 1, -1, -1, -1.0]) + vecs = np.concatenate([v_a, v_b])[:, np.newaxis] + + with patch('prody.dynamics.analysis._getModeVecs', return_value=vecs): + hinges = getGlobalHinges(object(), atoms=ag, threshold=15, + space=None, trim=False) + + # Expect a hinge from BOTH chains: global 2 (chain A) and 7 (chain B). + self.assertEqual(hinges, [[2, 7]]) + + +class TestGetGlobalHingesOffByOne(unittest.TestCase): + """Bug 1b: vecs[fst:lst] should be vecs[fst:lst+1] (inclusive chains).""" + + def test_hinge_at_last_residue_of_chain_is_detected(self): + # Single chain so the accumulation-indentation bug can't interfere + # (loop runs exactly once) -- isolates the slicing off-by-one. + ag = _make_two_chain_atoms(6, 0) # single 6-atom chain 'A' + # crossings at local (1,2) and (4,5); the second one needs the very + # last atom of the chain (global index 5) to be included in the slice. + v = np.array([-1, -1, 1, 1, 1, -1.0]) + vecs = v[:, np.newaxis] + + with patch('prody.dynamics.analysis._getModeVecs', return_value=vecs): + hinges = getGlobalHinges(object(), atoms=ag, threshold=15, + space=None, trim=False) + + self.assertEqual(hinges, [[1, 4]]) + + +class TestCalcCollectivityMasses(unittest.TestCase): + """Bug 3: masses path must divide by sum(u2in), not sqrt(sum(u2in)).""" + + def test_masses_path_matches_bruschweiler_definition(self): + v = np.array([0.6, 0.8]) + masses = np.array([1.0, 4.0]) + + u2 = v ** 2 / masses + p = u2 / u2.sum() + self.assertAlmostEqual(p.sum(), 1.0, places=12) + expected = np.exp(-(p * np.log(p)).sum()) / len(v) + + actual = calcCollectivity(v, masses=masses, is3d=False) + self.assertAlmostEqual(actual, expected, places=8) + + def test_default_path_without_masses_is_unaffected(self): + # Unit-norm vector: sum(u2in) == sqrt(sum(u2in)) == 1, so this path + # is identical before and after the fix. Guards against regressions. + v = np.array([0.6, 0.8]) + u2 = v ** 2 + p = u2 / u2.sum() + expected = np.exp(-(p * np.log(p)).sum()) / len(v) + + actual = calcCollectivity(v, is3d=False) + self.assertAlmostEqual(actual, expected, places=8) + + +if __name__ == '__main__': + unittest.main(verbosity=2) From b63f0a9f33849ba6d86c81e1318d0c809d18c6be Mon Sep 17 00:00:00 2001 From: Mike German Date: Wed, 12 Aug 2026 08:37:20 -0400 Subject: [PATCH 2/3] Rename the hinge/collectivity test module to test_hinges.py Per review: give the tests file a more generic name. Co-Authored-By: Claude --- .../dynamics/{test_analysis_hinge_bugs.py => test_hinges.py} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename prody/tests/dynamics/{test_analysis_hinge_bugs.py => test_hinges.py} (100%) diff --git a/prody/tests/dynamics/test_analysis_hinge_bugs.py b/prody/tests/dynamics/test_hinges.py similarity index 100% rename from prody/tests/dynamics/test_analysis_hinge_bugs.py rename to prody/tests/dynamics/test_hinges.py From 8302688e1f317d69e0583a5776e551213909a2f3 Mon Sep 17 00:00:00 2001 From: Mike German Date: Sat, 15 Aug 2026 11:22:19 -0400 Subject: [PATCH 3/3] Fold the hinge tests into test_hinge_finder.py, collectivity into its own module Per review: the hinge regressions belong with the existing hinge tests rather than in a new file, and collectivity is general enough to want a test_dynamics_analysis.py of its own for that kind of thing. No test bodies changed. All four still fail against the pre-fix analysis.py and pass after it. Co-Authored-By: Claude --- .../tests/dynamics/test_dynamics_analysis.py | 46 +++++++ prody/tests/dynamics/test_hinge_finder.py | 79 ++++++++++++ prody/tests/dynamics/test_hinges.py | 117 ------------------ 3 files changed, 125 insertions(+), 117 deletions(-) create mode 100644 prody/tests/dynamics/test_dynamics_analysis.py delete mode 100644 prody/tests/dynamics/test_hinges.py diff --git a/prody/tests/dynamics/test_dynamics_analysis.py b/prody/tests/dynamics/test_dynamics_analysis.py new file mode 100644 index 000000000..9438a8b45 --- /dev/null +++ b/prody/tests/dynamics/test_dynamics_analysis.py @@ -0,0 +1,46 @@ +"""This module contains unit tests for general functions in +:mod:`~prody.dynamics.analysis`, i.e. ones that are not tied to a +particular model or to a more specific test module.""" + +import numpy as np + +from prody import calcCollectivity, LOGGER +from prody.tests import unittest + +LOGGER.verbosity = 'none' + + +class TestCalcCollectivity(unittest.TestCase): + """The masses path must divide by ``sum(u2in)``, not ``sqrt(sum(u2in))``. + + Dividing by the square root breaks the probability-distribution + requirement of the collectivity definition in equation 5 of + Bruschweiler 1995. + """ + + def test_masses_path_matches_bruschweiler_definition(self): + v = np.array([0.6, 0.8]) + masses = np.array([1.0, 4.0]) + + u2 = v ** 2 / masses + p = u2 / u2.sum() + self.assertAlmostEqual(p.sum(), 1.0, places=12) + expected = np.exp(-(p * np.log(p)).sum()) / len(v) + + actual = calcCollectivity(v, masses=masses, is3d=False) + self.assertAlmostEqual(actual, expected, places=8) + + def test_default_path_without_masses_is_unaffected(self): + # Unit-norm vector: sum(u2in) == sqrt(sum(u2in)) == 1, so this path + # is identical before and after the fix. Guards against regressions. + v = np.array([0.6, 0.8]) + u2 = v ** 2 + p = u2 / u2.sum() + expected = np.exp(-(p * np.log(p)).sum()) / len(v) + + actual = calcCollectivity(v, is3d=False) + self.assertAlmostEqual(actual, expected, places=8) + + +if __name__ == '__main__': + unittest.main(verbosity=2) diff --git a/prody/tests/dynamics/test_hinge_finder.py b/prody/tests/dynamics/test_hinge_finder.py index fb84bf7f5..a747a2a56 100644 --- a/prody/tests/dynamics/test_hinge_finder.py +++ b/prody/tests/dynamics/test_hinge_finder.py @@ -1,10 +1,25 @@ import unittest +from unittest.mock import patch + import numpy as np from prody import * # TODO: Import your function here # from your_module_name import getGlobalHinges + +def _make_two_chain_atoms(n1, n2): + """Tiny synthetic 2-chain AtomGroup (CA-only) for hinge tests.""" + n = n1 + n2 + ag = AtomGroup('synthetic') + ag.setCoords(np.zeros((n, 3))) + ag.setChids(['A'] * n1 + ['B'] * n2) + ag.setResnums(list(range(1, n1 + 1)) + list(range(1, n2 + 1))) + ag.setNames(['CA'] * n) + ag.setResnames(['ALA'] * n) + return ag + + # --- THE UNIT TEST SUITE --- class TestHingesWithRealGNM(unittest.TestCase): @@ -80,6 +95,70 @@ def test_trim_functionality(self): self.assertGreater(len(hinges_trimmed), 0, "1AKE Mode 1 should have valid central hinges") +class TestGetHingesEmptyRegions(unittest.TestCase): + """getHinges must not crash when there is no sign change. + + ``regs`` ends up empty and the old code indexed ``regs[0]``. + """ + + def test_no_sign_change_returns_empty_list(self): + v = np.array([1.0, 1.0, 1.0, 1.0]) + # Should return [] (no hinges), not raise IndexError. + hinges = getHinges(v) + self.assertEqual(hinges, []) + + def test_normal_case_still_works(self): + # Sanity check the fix doesn't break the normal path. + v = np.array([-1, -1, -1, 1, 1, 1.0]) + self.assertEqual(getHinges(v), [2]) + + +class TestGetGlobalHingesChainAccumulation(unittest.TestCase): + """Hinges from all chains but the last were silently dropped. + + The accumulation block sat outside the per-chain loop. + """ + + def test_all_chains_contribute_hinges(self): + ag = _make_two_chain_atoms(6, 5) + + # Chain A (global idx 0-5): crossing well inside -> local hinge 2 + v_a = np.array([-1, -1, -1, 1, 1, 1.0]) + # Chain B (global idx 6-10): crossing well inside -> local hinge 1 + v_b = np.array([1, 1, -1, -1, -1.0]) + vecs = np.concatenate([v_a, v_b])[:, np.newaxis] + + with patch('prody.dynamics.analysis._getModeVecs', return_value=vecs): + hinges = getGlobalHinges(object(), atoms=ag, threshold=15, + space=None, trim=False) + + # Expect a hinge from BOTH chains: global 2 (chain A) and 7 (chain B). + self.assertEqual(hinges, [[2, 7]]) + + +class TestGetGlobalHingesOffByOne(unittest.TestCase): + """``vecs[fst:lst]`` should be ``vecs[fst:lst+1]``. + + ``chains`` stores inclusive (fst, lst) pairs, so the exclusive slice + dropped the last atom of every chain from consideration. + """ + + def test_hinge_at_last_residue_of_chain_is_detected(self): + # Single chain so the accumulation-indentation bug can't interfere + # (loop runs exactly once) -- isolates the slicing off-by-one. + ag = _make_two_chain_atoms(6, 0) # single 6-atom chain 'A' + # crossings at local (1,2) and (4,5); the second one needs the very + # last atom of the chain (global index 5) to be included in the slice. + v = np.array([-1, -1, 1, 1, 1, -1.0]) + vecs = v[:, np.newaxis] + + with patch('prody.dynamics.analysis._getModeVecs', return_value=vecs): + hinges = getGlobalHinges(object(), atoms=ag, threshold=15, + space=None, trim=False) + + self.assertEqual(hinges, [[1, 4]]) + + if __name__ == '__main__': # Verbosity=2 shows individual test status (OK/FAIL) unittest.main(verbosity=2) diff --git a/prody/tests/dynamics/test_hinges.py b/prody/tests/dynamics/test_hinges.py deleted file mode 100644 index 6c87e04eb..000000000 --- a/prody/tests/dynamics/test_hinges.py +++ /dev/null @@ -1,117 +0,0 @@ -"""Regression tests for three bugs found in prody.dynamics.analysis: - -1. getGlobalHinges silently drops hinges from every chain except the last - because the accumulation block is mis-indented outside the per-chain - loop, and also slices vecs with an exclusive upper bound (vecs[fst:lst]) - even though `chains` stores inclusive (fst, lst) pairs, dropping the - last atom of every chain from consideration. -2. getHinges raises IndexError when an eigenvector segment has no sign - change at all (regs ends up empty and `regs[0]` blows up). -3. calcCollectivity normalizes by dividing by sqrt(sum) instead of sum when - masses are supplied, which breaks the probability-distribution - requirement of the Bruschweiler 1995 (eq. 5) collectivity definition. -""" - -import unittest -from unittest.mock import patch - -import numpy as np -from prody import * -from prody.dynamics.analysis import getHinges, getGlobalHinges, calcCollectivity - - -def _make_two_chain_atoms(n1, n2): - """Tiny synthetic 2-chain AtomGroup (CA-only) for hinge tests.""" - n = n1 + n2 - ag = AtomGroup('synthetic') - ag.setCoords(np.zeros((n, 3))) - ag.setChids(['A'] * n1 + ['B'] * n2) - ag.setResnums(list(range(1, n1 + 1)) + list(range(1, n2 + 1))) - ag.setNames(['CA'] * n) - ag.setResnames(['ALA'] * n) - return ag - - -class TestGetHingesEmptyRegions(unittest.TestCase): - """Bug 2: getHinges must not crash when there is no sign change.""" - - def test_no_sign_change_returns_empty_list(self): - v = np.array([1.0, 1.0, 1.0, 1.0]) - # Should return [] (no hinges), not raise IndexError. - hinges = getHinges(v) - self.assertEqual(hinges, []) - - def test_normal_case_still_works(self): - # Sanity check the fix doesn't break the normal path. - v = np.array([-1, -1, -1, 1, 1, 1.0]) - self.assertEqual(getHinges(v), [2]) - - -class TestGetGlobalHingesChainAccumulation(unittest.TestCase): - """Bug 1a: hinges from all chains but the last are silently dropped.""" - - def test_all_chains_contribute_hinges(self): - ag = _make_two_chain_atoms(6, 5) - - # Chain A (global idx 0-5): crossing well inside -> local hinge 2 - v_a = np.array([-1, -1, -1, 1, 1, 1.0]) - # Chain B (global idx 6-10): crossing well inside -> local hinge 1 - v_b = np.array([1, 1, -1, -1, -1.0]) - vecs = np.concatenate([v_a, v_b])[:, np.newaxis] - - with patch('prody.dynamics.analysis._getModeVecs', return_value=vecs): - hinges = getGlobalHinges(object(), atoms=ag, threshold=15, - space=None, trim=False) - - # Expect a hinge from BOTH chains: global 2 (chain A) and 7 (chain B). - self.assertEqual(hinges, [[2, 7]]) - - -class TestGetGlobalHingesOffByOne(unittest.TestCase): - """Bug 1b: vecs[fst:lst] should be vecs[fst:lst+1] (inclusive chains).""" - - def test_hinge_at_last_residue_of_chain_is_detected(self): - # Single chain so the accumulation-indentation bug can't interfere - # (loop runs exactly once) -- isolates the slicing off-by-one. - ag = _make_two_chain_atoms(6, 0) # single 6-atom chain 'A' - # crossings at local (1,2) and (4,5); the second one needs the very - # last atom of the chain (global index 5) to be included in the slice. - v = np.array([-1, -1, 1, 1, 1, -1.0]) - vecs = v[:, np.newaxis] - - with patch('prody.dynamics.analysis._getModeVecs', return_value=vecs): - hinges = getGlobalHinges(object(), atoms=ag, threshold=15, - space=None, trim=False) - - self.assertEqual(hinges, [[1, 4]]) - - -class TestCalcCollectivityMasses(unittest.TestCase): - """Bug 3: masses path must divide by sum(u2in), not sqrt(sum(u2in)).""" - - def test_masses_path_matches_bruschweiler_definition(self): - v = np.array([0.6, 0.8]) - masses = np.array([1.0, 4.0]) - - u2 = v ** 2 / masses - p = u2 / u2.sum() - self.assertAlmostEqual(p.sum(), 1.0, places=12) - expected = np.exp(-(p * np.log(p)).sum()) / len(v) - - actual = calcCollectivity(v, masses=masses, is3d=False) - self.assertAlmostEqual(actual, expected, places=8) - - def test_default_path_without_masses_is_unaffected(self): - # Unit-norm vector: sum(u2in) == sqrt(sum(u2in)) == 1, so this path - # is identical before and after the fix. Guards against regressions. - v = np.array([0.6, 0.8]) - u2 = v ** 2 - p = u2 / u2.sum() - expected = np.exp(-(p * np.log(p)).sum()) / len(v) - - actual = calcCollectivity(v, is3d=False) - self.assertAlmostEqual(actual, expected, places=8) - - -if __name__ == '__main__': - unittest.main(verbosity=2)