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 -- wasn't possible to use `frames=...` syntax before.
marinegor marked this conversation as resolved.
Show resolved Hide resolved
* Fix deploy action to use the correct version of the pypi upload action.

Enhancements
Expand Down
10 changes: 8 additions & 2 deletions package/MDAnalysis/analysis/pca.py
Original file line number Diff line number Diff line change
Expand Up @@ -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``.
marinegor marked this conversation as resolved.
Show resolved Hide resolved
It is now fixed.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
It is now fixed.

"""

def __init__(self, universe, select='all', align=False, mean=None,
Expand All @@ -247,7 +253,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 +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()
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).run(frames=[0, 1])
marinegor marked this conversation as resolved.
Show resolved Hide resolved


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