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.

Sign up for GitHub

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

Jump to bottom

Added tqdm bar for: pca.transform() #4531

Merged
merged 11 commits into from
Mar 28, 2024
Merged

Added tqdm bar for: pca.transform() #4531

Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
3601e1a
Added tqdm bar for: pca.transform()
Mar 25, 2024
1b157ca
Fixed as per comments on PR #4531
Mar 26, 2024
1fe9e8c
Fixes:round2 as per comments on PR MDAnalysis#4531
Mar 27, 2024
7d63aa8
Updated fixes for #4531
Mar 27, 2024
446de50
Merge branch 'develop' into pca_transform_progress
SampurnaM Mar 28, 2024
04f801a
Final edits
Mar 28, 2024
0b8725f
Updated CHANGELOG : merge conflict resolved
SampurnaM Mar 28, 2024
51190d2
Updated log.py to remove extra class for ProgressBarDual
SampurnaM Mar 28, 2024
9103f19
Update package/CHANGELOG
orbeckst Mar 28, 2024
cd4850d
Update package/CHANGELOG
SampurnaM Mar 28, 2024
92d5541
Update pca.py
SampurnaM Mar 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 16 additions & 6 deletions package/MDAnalysis/analysis/pca.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,11 +121,12 @@

from MDAnalysis import Universe
from MDAnalysis.analysis.align import _fit_to
from MDAnalysis.lib.log import ProgressBar
from MDAnalysis.lib.log import ProgressBar, ProgressBar_Dual
SampurnaM marked this conversation as resolved.
Show resolved Hide resolved

from ..lib import util
from ..due import due, Doi
from .base import AnalysisBase
from tqdm.auto import tqdm
Copy link
Member

Choose a reason for hiding this comment

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

move imports of other packages above the MDA imports

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.



class PCA(AnalysisBase):
Expand Down Expand Up @@ -355,12 +356,12 @@ def n_components(self, n):
n = len(self._variance)
self.results.variance = self._variance[:n]
self.results.cumulated_variance = (np.cumsum(self._variance) /
np.sum(self._variance))[:n]
np.sum(self._variance))[:n]
orbeckst marked this conversation as resolved.
Show resolved Hide resolved
self.results.p_components = self._p_components[:, :n]
self._n_components = n

def transform(self, atomgroup, n_components=None, start=None, stop=None,
step=None):
step=None, verbose=None):
Copy link
Member

Choose a reason for hiding this comment

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

Set the default to False because it's not clear what None will do from the docs. If it behaves like False then be clear and set it to False in the first place.

"""Apply the dimensionality reduction on a trajectory

Parameters
Expand All @@ -382,6 +383,7 @@ def transform(self, atomgroup, n_components=None, start=None, stop=None,
Include every `step` frames in the PCA transform. If set to
``None`` (the default) then every frame is analyzed (i.e., same as
``step=1``).
verbose: Boolean,optional
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Add a .. versionadded:: 2.8.0 to this new kwarg (see other doc strings for how to do it).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Thanks for the detailed guidance!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, added now :)


Returns
-------
Expand All @@ -398,7 +400,7 @@ def transform(self, atomgroup, n_components=None, start=None, stop=None,
if isinstance(atomgroup, Universe):
atomgroup = atomgroup.atoms

if(self._n_atoms != atomgroup.n_atoms):
if (self._n_atoms != atomgroup.n_atoms):
Copy link
Member

Choose a reason for hiding this comment

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

This formatting change is ok as it was really, really ugly ;-).

Also remove the parentheses.

Suggested change
if (self._n_atoms != atomgroup.n_atoms):
if self._n_atoms != atomgroup.n_atoms:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

raise ValueError('PCA has been fit for'
'{} atoms. Your atomgroup'
'has {} atoms'.format(self._n_atoms,
Expand All @@ -415,7 +417,15 @@ def transform(self, atomgroup, n_components=None, start=None, stop=None,

dot = np.zeros((n_frames, dim))

for i, ts in enumerate(traj[start:stop:step]):
if verbose == True:
self.disabled = False
else:
self.disabled = True
Copy link
Member

Choose a reason for hiding this comment

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

Why do you need to store the value in the new attribute self.disabled?

I think you'll only ever use it in this function.

So unless there are really important reasons (please explain!), remove self.disabled.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was initially going to check if there were any other open issues for adding ProgressBar/verbose option in PCA, so I saved it so we can reuse it if needed, but you are right, it doesn't seem so. Removed now.


# for i, ts in tqdm(enumerate(traj[start:stop:step]), desc="Transform progress"):
SampurnaM marked this conversation as resolved.
Show resolved Hide resolved
for i, ts in tqdm(enumerate(traj[start:stop:step]), disable=self.disabled, desc="Transform progress"):
Copy link
Member

Choose a reason for hiding this comment

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

Just write

disable=not verbose

Copy link
Member

Choose a reason for hiding this comment

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

Check PEP8: this line almost certainly needs to be broken

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Apologies. Done now.

# print("Iteration", i, "Timestep",ts)
# verbose=self._verbose, desc="Transform progress"):
Copy link
Member

Choose a reason for hiding this comment

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

remove commented out code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

xyz = atomgroup.positions.ravel() - self._xmean
dot[i] = np.dot(xyz, self._p_components[:, :dim])

Expand Down Expand Up @@ -561,7 +571,7 @@ def project_single_frame(self, components=None, group=None, anchor=None):
for res in group.residues:
# n_common is the number of pca atoms in a residue
n_common = pca_res_counts[np.where(
pca_res_indices == res.resindex)][0]
pca_res_indices == res.resindex)][0]
Copy link
Member

Choose a reason for hiding this comment

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

Don't format other code — it makes the diff noisy.

Copy link
Member

Choose a reason for hiding this comment

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

Undo this one, autopep8 is less clear than the original.

non_pca_atoms = np.append(non_pca_atoms,
res.atoms.n_atoms - n_common)
# index_extrapolate records the anchor number for each non-PCA atom
Expand Down
Loading