From 211cbcf6a64ed932641acb869ea642ccd6a42fca Mon Sep 17 00:00:00 2001 From: Egor Marin Date: Fri, 12 Jan 2024 15:17:54 +0100 Subject: [PATCH 01/10] Fix bug in PCA trajectory iteration -- avoid explicit usage of self.start --- package/MDAnalysis/analysis/pca.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/package/MDAnalysis/analysis/pca.py b/package/MDAnalysis/analysis/pca.py index cad6946b2dc..fef2ac9a79a 100644 --- a/package/MDAnalysis/analysis/pca.py +++ b/package/MDAnalysis/analysis/pca.py @@ -247,7 +247,8 @@ def __init__(self, universe, select='all', align=False, mean=None, def _prepare(self): # access start index - self._u.trajectory[self.start] + # self._u.trajectory[self.start] + self._sliced_trajectory[0] # reference will be start index self._reference = self._u.select_atoms(self._select) self._atoms = self._u.select_atoms(self._select) From d4de9107c563ac20ed778dcb2a84ab017b7909f5 Mon Sep 17 00:00:00 2001 From: Egor Marin Date: Fri, 12 Jan 2024 15:22:41 +0100 Subject: [PATCH 02/10] update changelog and tests for PCA fix --- package/CHANGELOG | 1 + testsuite/MDAnalysisTests/analysis/test_pca.py | 5 +++++ 2 files changed, 6 insertions(+) diff --git a/package/CHANGELOG b/package/CHANGELOG index 142db2aa8c4..94cf984e04b 100644 --- a/package/CHANGELOG +++ b/package/CHANGELOG @@ -19,6 +19,7 @@ The rules for this file: * 2.8.0 Fixes + * Fix bug in PCA -- wasn't possible to use `frames=...` syntax before. * Fix deploy action to use the correct version of the pypi upload action. Enhancements diff --git a/testsuite/MDAnalysisTests/analysis/test_pca.py b/testsuite/MDAnalysisTests/analysis/test_pca.py index e157bee994e..b001ac4a8f3 100644 --- a/testsuite/MDAnalysisTests/analysis/test_pca.py +++ b/testsuite/MDAnalysisTests/analysis/test_pca.py @@ -132,6 +132,11 @@ def test_no_frames(u): with pytest.raises(ValueError): PCA(u, select=SELECTION).run(stop=1) +def test_can_run_frames(u): + atoms = u.select_atoms(SELECTION) + u.transfer_to_memory() + PCA(u, select=SELECTION).run(frames=[0,1]) + def test_transform(pca, u): ag = u.select_atoms(SELECTION) From be05e9ba62c071df963da376abe32dd23c345cd5 Mon Sep 17 00:00:00 2001 From: Egor Marin Date: Fri, 12 Jan 2024 17:29:19 +0300 Subject: [PATCH 03/10] Update test_pca.py fix formatting changes --- testsuite/MDAnalysisTests/analysis/test_pca.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/testsuite/MDAnalysisTests/analysis/test_pca.py b/testsuite/MDAnalysisTests/analysis/test_pca.py index b001ac4a8f3..9007eed543c 100644 --- a/testsuite/MDAnalysisTests/analysis/test_pca.py +++ b/testsuite/MDAnalysisTests/analysis/test_pca.py @@ -132,10 +132,11 @@ def test_no_frames(u): with pytest.raises(ValueError): PCA(u, select=SELECTION).run(stop=1) + def test_can_run_frames(u): atoms = u.select_atoms(SELECTION) u.transfer_to_memory() - PCA(u, select=SELECTION).run(frames=[0,1]) + PCA(u, select=SELECTION).run(frames=[0, 1]) def test_transform(pca, u): From 9743827b3a8dd710d5ea11da7c87d77c248aa0d2 Mon Sep 17 00:00:00 2001 From: Egor Marin Date: Wed, 17 Jan 2024 21:53:09 +0100 Subject: [PATCH 04/10] Replace self._u.trajectory[...] with self._sliced_trajectory --- package/MDAnalysis/analysis/pca.py | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/package/MDAnalysis/analysis/pca.py b/package/MDAnalysis/analysis/pca.py index fef2ac9a79a..3c6c739915d 100644 --- a/package/MDAnalysis/analysis/pca.py +++ b/package/MDAnalysis/analysis/pca.py @@ -230,6 +230,12 @@ class PCA(AnalysisBase): ``mean`` input now accepts coordinate arrays instead of atomgroup. :attr:`p_components`, :attr:`variance` and :attr:`cumulated_variance` are now stored in a :class:`MDAnalysis.analysis.base.Results` instance. + .. versionchanged:: 2.8.0 + ``self.run()`` can now appropriately use ``frames`` parameter (bug + described by #4425 and fixed by #4423). Previously, behaviour was to + manually iterate through ``self._trajectory``, which starting #3415 that + introduced ``frames`` is wrong, and should use ``self._sliced_trajectory``. + It is now fixed. """ def __init__(self, universe, select='all', align=False, mean=None, @@ -247,7 +253,6 @@ def __init__(self, universe, select='all', align=False, mean=None, def _prepare(self): # access start index - # self._u.trajectory[self.start] self._sliced_trajectory[0] # reference will be start index self._reference = self._u.select_atoms(self._select) @@ -276,7 +281,7 @@ def _prepare(self): self._ref_atom_positions -= self._ref_cog if self._calc_mean: - for ts in ProgressBar(self._u.trajectory[self.start:self.stop:self.step], + for ts in ProgressBar(self._sliced_trajectory, verbose=self._verbose, desc="Mean Calculation"): if self.align: mobile_cog = self._atoms.center_of_geometry() From e14686369a97b74cf1da60d939c7252c84c4d705 Mon Sep 17 00:00:00 2001 From: Egor Marin Date: Wed, 17 Jan 2024 23:24:18 +0100 Subject: [PATCH 05/10] Update documentaiton --- package/MDAnalysis/analysis/pca.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/package/MDAnalysis/analysis/pca.py b/package/MDAnalysis/analysis/pca.py index 3c6c739915d..c0ae0fee6e1 100644 --- a/package/MDAnalysis/analysis/pca.py +++ b/package/MDAnalysis/analysis/pca.py @@ -142,7 +142,8 @@ class PCA(AnalysisBase): generates the principal components of the backbone of the atomgroup and then transforms those atomgroup coordinates by the direction of those variances. Please refer to the :ref:`PCA-tutorial` for more detailed - instructions. + instructions. When using mean selections, the first frame of the selected + trajectory slice is used as a reference. Parameters ---------- @@ -235,7 +236,6 @@ class PCA(AnalysisBase): described by #4425 and fixed by #4423). Previously, behaviour was to manually iterate through ``self._trajectory``, which starting #3415 that introduced ``frames`` is wrong, and should use ``self._sliced_trajectory``. - It is now fixed. """ def __init__(self, universe, select='all', align=False, mean=None, From a4b2f4ad098835b274a352a51579688765adaa84 Mon Sep 17 00:00:00 2001 From: Egor Marin Date: Thu, 18 Jan 2024 18:26:11 +0300 Subject: [PATCH 06/10] Update testsuite/MDAnalysisTests/analysis/test_pca.py Co-authored-by: Irfan Alibay --- testsuite/MDAnalysisTests/analysis/test_pca.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/testsuite/MDAnalysisTests/analysis/test_pca.py b/testsuite/MDAnalysisTests/analysis/test_pca.py index 9007eed543c..0841917ccf6 100644 --- a/testsuite/MDAnalysisTests/analysis/test_pca.py +++ b/testsuite/MDAnalysisTests/analysis/test_pca.py @@ -136,7 +136,7 @@ def test_no_frames(u): def test_can_run_frames(u): atoms = u.select_atoms(SELECTION) u.transfer_to_memory() - PCA(u, select=SELECTION).run(frames=[0, 1]) + PCA(u, select=SELECTION, mean=True).run(frames=[0, 1]) def test_transform(pca, u): From 0c95e34a7b444d273774e876a94abd627983328c Mon Sep 17 00:00:00 2001 From: Egor Marin Date: Thu, 18 Jan 2024 18:26:43 +0300 Subject: [PATCH 07/10] Update package/MDAnalysis/analysis/pca.py Co-authored-by: Irfan Alibay --- package/MDAnalysis/analysis/pca.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/package/MDAnalysis/analysis/pca.py b/package/MDAnalysis/analysis/pca.py index c0ae0fee6e1..4ed5486e4c6 100644 --- a/package/MDAnalysis/analysis/pca.py +++ b/package/MDAnalysis/analysis/pca.py @@ -234,8 +234,9 @@ class PCA(AnalysisBase): .. versionchanged:: 2.8.0 ``self.run()`` can now appropriately use ``frames`` parameter (bug described by #4425 and fixed by #4423). Previously, behaviour was to - manually iterate through ``self._trajectory``, which starting #3415 that - introduced ``frames`` is wrong, and should use ``self._sliced_trajectory``. + manually iterate through ``self._trajectory``, which would + incorrectly handle cases where the ``frame`` argument + was passed. """ def __init__(self, universe, select='all', align=False, mean=None, From 39965ee693e703f3cdf3f09ddbebecd4cdddfebe Mon Sep 17 00:00:00 2001 From: Egor Marin Date: Thu, 18 Jan 2024 18:27:01 +0300 Subject: [PATCH 08/10] Update package/CHANGELOG Co-authored-by: Irfan Alibay --- package/CHANGELOG | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package/CHANGELOG b/package/CHANGELOG index 9114d9f3b99..b87116b2940 100644 --- a/package/CHANGELOG +++ b/package/CHANGELOG @@ -19,7 +19,7 @@ The rules for this file: * 2.8.0 Fixes - * Fix bug in PCA -- wasn't possible to use `frames=...` syntax before. + * Fix bug in PCA preventing use of `frames=...` syntax (PR #4423) * Fix `analysis/diffusionmap.py` iteration through trajectory to iteration over `self._sliced_trajectory`, hence supporting `DistanceMatrix.run(frames=...)` (PR #4433) From 3b270c821d91949587b7c70ab0902529683d9f88 Mon Sep 17 00:00:00 2001 From: Egor Marin Date: Thu, 18 Jan 2024 17:38:09 +0100 Subject: [PATCH 09/10] revert back to testing with mean=None --- testsuite/MDAnalysisTests/analysis/test_pca.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/testsuite/MDAnalysisTests/analysis/test_pca.py b/testsuite/MDAnalysisTests/analysis/test_pca.py index 0841917ccf6..9007eed543c 100644 --- a/testsuite/MDAnalysisTests/analysis/test_pca.py +++ b/testsuite/MDAnalysisTests/analysis/test_pca.py @@ -136,7 +136,7 @@ def test_no_frames(u): def test_can_run_frames(u): atoms = u.select_atoms(SELECTION) u.transfer_to_memory() - PCA(u, select=SELECTION, mean=True).run(frames=[0, 1]) + PCA(u, select=SELECTION).run(frames=[0, 1]) def test_transform(pca, u): From 7e915a4bc02504efdda442a305c20512dbe23da3 Mon Sep 17 00:00:00 2001 From: Egor Marin Date: Thu, 18 Jan 2024 17:56:10 +0100 Subject: [PATCH 10/10] Enforce mean=None when testing with frames --- testsuite/MDAnalysisTests/analysis/test_pca.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/testsuite/MDAnalysisTests/analysis/test_pca.py b/testsuite/MDAnalysisTests/analysis/test_pca.py index 9007eed543c..c72971c30ed 100644 --- a/testsuite/MDAnalysisTests/analysis/test_pca.py +++ b/testsuite/MDAnalysisTests/analysis/test_pca.py @@ -136,7 +136,7 @@ def test_no_frames(u): def test_can_run_frames(u): atoms = u.select_atoms(SELECTION) u.transfer_to_memory() - PCA(u, select=SELECTION).run(frames=[0, 1]) + PCA(u, select=SELECTION, mean=None).run(frames=[0, 1]) def test_transform(pca, u):