Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix bug in PCA trajectory iteration -- avoid explicit usage of self.start #4423

Merged
merged 11 commits into from
Jan 20, 2024
1 change: 1 addition & 0 deletions package/CHANGELOG
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ The rules for this file:
* 2.8.0

Fixes
* 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)
Expand Down
13 changes: 10 additions & 3 deletions package/MDAnalysis/analysis/pca.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is great thanks!

trajectory slice is used as a reference.

Parameters
----------
Expand Down Expand Up @@ -230,6 +231,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 would
incorrectly handle cases where the ``frame`` argument
was passed.
"""

def __init__(self, universe, select='all', align=False, mean=None,
Expand All @@ -247,7 +254,7 @@ 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]
IAlibay marked this conversation as resolved.
Show resolved Hide resolved
# reference will be start index
self._reference = self._u.select_atoms(self._select)
self._atoms = self._u.select_atoms(self._select)
Expand Down Expand Up @@ -275,7 +282,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()
Expand Down
6 changes: 6 additions & 0 deletions testsuite/MDAnalysisTests/analysis/test_pca.py
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,12 @@ def test_no_frames(u):
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, mean=None).run(frames=[0, 1])


def test_transform(pca, u):
ag = u.select_atoms(SELECTION)
pca_space = pca.transform(ag, n_components=1)
Expand Down
Loading