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

SampurnaM · 2024-03-25T16:26:34Z

Fixes #3773

Changes made in this Pull Request:

Added an tqdm function over pca.transform() iterations, so that verbose = True shows a progress bar like pca.PCA.run() ( As per comments in the issue)
I made a separate class called ProgressDual for ProgressBar with two iterables instead of one, but feels like an overkill so haven't added/pushed to this commit.

( I am very much a beginner, please let me know where I have messed up/ skipped something obvious!)

PR Checklist

Tests? Locally
Docs? Will edit if this PR passes automated checks
CHANGELOG updated? Will edit if this PR passes automated checks
Issue raised/referenced? PCA not showing progress bar on mean structure calculation #3773

Developers certificate of origin

I certify that this contribution is covered by the LGPLv2.1+ license as defined in our LICENSE and adheres to the Developer Certificate of Origin.

pep8speaks · 2024-03-25T16:26:40Z

Hello @SampurnaM! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

In the file package/MDAnalysis/analysis/pca.py:

Line 427:80: E501 line too long (80 > 79 characters)

Comment last updated at 2024-03-28 19:08:38 UTC

github-actions · 2024-03-25T16:29:00Z

Linter Bot Results:

Hi @SampurnaM! Thanks for making this PR. We linted your code and found the following:

Some issues were found with the formatting of your code.

Code Location	Outcome
main package	⚠️ Possible failure
testsuite	✅ Passed

Please have a look at the darker-main-code and darker-test-code steps here for more details: https://github.com/MDAnalysis/mdanalysis/actions/runs/8472460092/job/23214620251

Please note: The black linter is purely informational, you can safely ignore these outcomes if there are no flake8 failures!

orbeckst · 2024-03-25T16:36:15Z

Thank you for your contribution. I edited your comments so that the PR is properly linked to issue #3773 and use standard notation for the check boxes.

orbeckst

Thank you for your contribution.

Please address my inline comments and

add yourself to AUTHORS
add an entry to CHANGELOG

I am not 100% sure if we test progress bars so there may be additional requests for tests. However, to demonstrate that it works for you, please run your code yourself and attach a screenshot to this issue in which you show what it looks like with the new progress bar.

orbeckst · 2024-03-25T16:45:27Z

package/MDAnalysis/analysis/pca.py

+        if verbose == True:
+            self.disabled = False
+        else:
+            self.disabled = True


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.

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.

package/MDAnalysis/analysis/pca.py

orbeckst · 2024-03-25T16:38:58Z

package/MDAnalysis/analysis/pca.py

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


remove commented out code

package/MDAnalysis/analysis/pca.py

orbeckst · 2024-03-25T16:40:11Z

package/MDAnalysis/analysis/pca.py


 from ..lib import util
 from ..due import due, Doi
 from .base import AnalysisBase
+from tqdm.auto import tqdm


move imports of other packages above the MDA imports

orbeckst · 2024-03-25T16:42:04Z

package/MDAnalysis/analysis/pca.py

@@ -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


space after comma ... and check if this how we indicate optional arguments (I don't remember) — see https://userguide.mdanalysis.org/stable/contributing_code.html#working-with-the-code-documentation and the links therein for numpy-docs and napoleon

add a one-line explanation

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

Done. Thanks for the detailed guidance!

Thanks, added now :)

orbeckst · 2024-03-25T16:46:07Z

package/MDAnalysis/analysis/pca.py

+            self.disabled = True
+
+        # for i, ts in tqdm(enumerate(traj[start:stop:step]), desc="Transform progress"):
+        for i, ts in tqdm(enumerate(traj[start:stop:step]), disable=self.disabled, desc="Transform progress"):


Just write

disable=not verbose

Check PEP8: this line almost certainly needs to be broken

Apologies. Done now.

orbeckst · 2024-03-25T16:47:38Z

package/MDAnalysis/analysis/pca.py

@@ -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]


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

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

package/MDAnalysis/analysis/pca.py

orbeckst · 2024-03-25T16:49:14Z

package/MDAnalysis/analysis/pca.py

@@ -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):


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:

codecov · 2024-03-26T14:33:09Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.63%. Comparing base (f5335b4) to head (92d5541).

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #4531      +/-   ##
===========================================
- Coverage    93.65%   93.63%   -0.03%     
===========================================
  Files          168      180      +12     
  Lines        21215    22295    +1080     
  Branches      3908     3908              
===========================================
+ Hits         19869    20876    +1007     
- Misses         888      961      +73     
  Partials       458      458

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

orbeckst

Looking good, just a few fixes (see comments) and please post a screen shot showing that it works locally.

orbeckst · 2024-03-26T15:35:48Z

package/CHANGELOG

+03/26/24 orbeckst, SampurnaM
+
+ * 2.8.0
+
+Fixes:
+ * Added a tqdm progress bar for ``MDAnalysis.analysis.pca.PCA.transform()``
+    It can be displayed or disabled using ``verbose`` parameter 
+    ``True = display | False = display off``
+     (PR #4531)
+
+


Add everything to the existing 2.8.0 block: It should look like

??/??/?? IAlibay, HeetVekariya, marinegor, lilyminium, RMeli, ljwoods2, aditya292002, pstaerk, PicoCentauri, BFedder. SampurnaM * 2.8.0

and then add under Enhancements

* Added a tqdm progress bar for `MDAnalysis.analysis.pca.PCA.transform()` (PR #4531)

(Minimal detail is sufficient, and break lines a 79 characters)

package/MDAnalysis/analysis/pca.py

orbeckst · 2024-03-26T15:38:03Z

package/MDAnalysis/analysis/pca.py

            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):


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.

orbeckst · 2024-03-26T15:38:42Z

package/MDAnalysis/analysis/pca.py

@@ -391,14 +397,17 @@ def transform(self, atomgroup, n_components=None, start=None, stop=None,
        .. versionchanged:: 0.19.0
           Transform now requires that :meth:`run` has been called before,
           otherwise a :exc:`ValueError` is raised.
+        .. versionadded:: 2.8.0
+           Transform now has shows a tqdm progressbar, which can be toggled
+           on with verbose = True, or off with verbose = False


When adding to the changes at the bottom of the class, this must be .. versionchanged:: 2.8.0 because there's only one time that the whole class was added. (Alternatively one can add versionchanged/added for individual blocks – see docs on numpy docs/sphinx). The way you're doing is her is fine, just change to versionchanged.

you can use markup
on with ``verbose = True``, or off with ``verbose = False``

orbeckst · 2024-03-26T15:42:51Z

package/MDAnalysis/analysis/pca.py

@@ -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]


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

SampurnaM · 2024-03-27T10:53:57Z

Also adding the screenshots here in case somebody else wants to have look in the future:

SampurnaM · 2024-03-27T11:04:07Z

Oh no.
I mistakenly added the package/MDAnalysis/lib/log.py to the last commit, and pushed the commit before running darker. Sorry.

What I did to reverse the commit:

git reset HEAD~
Unstaged changes after reset:
M package/CHANGELOG
M package/MDAnalysis/analysis/pca.py
M package/MDAnalysis/lib/log.py
git rebase -i HEAD~1
error: cannot rebase: You have unstaged changes.
error: Please commit or stash them.
git add package/MDAnalysis/analysis/pca.py
git add package/CHANGELOG
git commit -m "Updated fixes for #4531"
[pca_transform_progress 7d63aa8] Updated fixes for #4531
Committer: sampurna <sampurna@JoyRamkrishna.>
Your name and email address were configured automatically based
on your username and hostname. Please check that they are accurate.
You can suppress this message by setting them explicitly:

git config --global user.name "Your Name"
git config --global user.email [email protected]

After doing this, you may fix the identity used for this commit with:

git commit --amend --reset-author

2 files changed, 11 insertions(+), 19 deletions(-)
git rebase -i 1fe9e8cfc~1
error: cannot rebase: You have unstaged changes.
error: Please commit or stash them.
git push origin pca_transform_progress
To https://github.com/SampurnaM/mdanalysis.git
! [rejected] pca_transform_progress -> pca_transform_progress (non-fast-forward)
error: failed to push some refs to 'https://github.com/SampurnaM/mdanalysis.git'
hint: Updates were rejected because the tip of your current branch is behind
hint: its remote counterpart. If you want to integrate the remote changes,
hint: use 'git pull' before pushing again.
hint: See the 'Note about fast-forwards' in 'git push --help' for details.

##Issue on autopep8:
I tried to reformat line 574 on pca.py using Notepad to reverse the auto formatting.
darker --diff -r upstream/develop package/MDAnalysis -L flake8

package/MDAnalysis/analysis/pca.py:576:13: E101 indentation contains mixed spaces and tabs [flake8]

package/MDAnalysis/authors.py:1:1: E265 block comment should start with '# ' [flake8]

package/MDAnalysis/core/PR_prep_driver_code_sm_24_3_24.py:1:1: E265 block comment should start with '# ' [flake8]

And now there is a conflict in package/AUTHORS, so I can't push the new commit :(

package/MDAnalysis/core/PR_prep_driver_code_sm_24_3_24.py:3:1: F401 'MDAnalysis.tests.datafiles.NCDF' imported but unused [flake8]
package/MDAnalysis/core/PR_prep_driver_code_sm_24_3_24.py:3:89: E501 line too long (89 > 88 characters) [flake8]

orbeckst · 2024-03-27T15:56:44Z

On GitHub there's a "Resolve conflicts" button that is a quick way to resolve simple conflicts. Try using it , see https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/addressing-merge-conflicts/resolving-a-merge-conflict-on-github for help. For CHANGELOG, just rearrange things so that your new entry is at the top of the subsection and that your user handle is at the end of the contributors list for the release.

orbeckst · 2024-03-27T15:58:02Z

Don't worry about keeping the history of the PR pretty, just push commits as they come. We will squash-merge everything into a single commit on merge.

orbeckst · 2024-03-27T15:59:29Z

Please ping me with @orbeckst when you think the PR is ready for a possibly final review.

Progressbar using tqdm() has been approved, don't need an entire new class.

SampurnaM · 2024-03-28T16:44:08Z

@orbeckst : I think I am ready for the final review. Please let me know :)

IAlibay

Couple of quick comments - edit: whomever reviews next please feel free to discard my review!

package/CHANGELOG

IAlibay · 2024-03-28T17:39:02Z

package/MDAnalysis/analysis/pca.py

@@ -561,7 +573,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]


This line contains a tab space, could you change this to only be standard spaces please?

How did you notice?!

But yes, no TABS!!!

How did you notice?!

The honest answer is that the formatting was bothering me (I usually newline at the [ and then do a 4 space identation, etc...), so I was dragging along to count the number of spaces and my mouse jumped..

orbeckst

Looks good to me. Nice work!

Please address @IAlibay 's comments.

package/CHANGELOG

orbeckst · 2024-03-28T17:52:50Z

package/MDAnalysis/analysis/pca.py

@@ -561,7 +573,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]


How did you notice?!

But yes, no TABS!!!

Co-authored-by: Irfan Alibay <[email protected]>

orbeckst · 2024-03-28T21:54:16Z

Congratulations @SampurnaM on your first merged PR! Thank you for your contribution!

Added tqdm bar for: pca.transform()

3601e1a

github-actions bot added the Component-Analysis label Mar 25, 2024

SampurnaM mentioned this pull request Mar 25, 2024

PCA not showing progress bar on mean structure calculation #3773

Closed

orbeckst requested changes Mar 25, 2024

View reviewed changes

SampurnaM mentioned this pull request Mar 26, 2024

[WIP]Bug fixing attempts for GSoC 2024 #4530

Closed

1 task

Fixed as per comments on PR MDAnalysis#4531

1b157ca

orbeckst requested changes Mar 26, 2024

View reviewed changes

orbeckst self-assigned this Mar 26, 2024

Fixes:round2 as per comments on PR MDAnalysis#4531

1fe9e8c

github-actions bot added the Component-lib label Mar 27, 2024

Updated fixes for MDAnalysis#4531

7d63aa8

SampurnaM and others added 4 commits March 28, 2024 14:14

Merge branch 'develop' into pca_transform_progress

446de50

Final edits

04f801a

Updated CHANGELOG : merge conflict resolved

0b8725f

Updated log.py to remove extra class for ProgressBarDual

51190d2

Progressbar using tqdm() has been approved, don't need an entire new class.

IAlibay requested changes Mar 28, 2024

View reviewed changes

Update package/CHANGELOG

9103f19

orbeckst approved these changes Mar 28, 2024

View reviewed changes

SampurnaM and others added 2 commits March 29, 2024 00:32

Update package/CHANGELOG

cd4850d

Co-authored-by: Irfan Alibay <[email protected]>

Update pca.py

92d5541

IAlibay approved these changes Mar 28, 2024

View reviewed changes

IAlibay merged commit dfcd06f into MDAnalysis:develop Mar 28, 2024
21 of 23 checks passed

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

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

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

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

Conversation

SampurnaM commented Mar 25, 2024 • edited by orbeckst Loading

PR Checklist

Developers certificate of origin

pep8speaks commented Mar 25, 2024 • edited Loading

Comment last updated at 2024-03-28 19:08:38 UTC

github-actions bot commented Mar 25, 2024 • edited Loading

Linter Bot Results:

orbeckst commented Mar 25, 2024

orbeckst left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Mar 26, 2024 • edited Loading

Codecov Report

orbeckst left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SampurnaM commented Mar 27, 2024

Also adding the screenshots here in case somebody else wants to have look in the future:

SampurnaM commented Mar 27, 2024

What I did to reverse the commit:

And now there is a conflict in package/AUTHORS, so I can't push the new commit :(

orbeckst commented Mar 27, 2024

orbeckst commented Mar 27, 2024

orbeckst commented Mar 27, 2024

SampurnaM commented Mar 28, 2024

IAlibay left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

orbeckst left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

orbeckst commented Mar 28, 2024

SampurnaM commented Mar 25, 2024 •

edited by orbeckst

Loading

pep8speaks commented Mar 25, 2024 •

edited

Loading

github-actions bot commented Mar 25, 2024 •

edited

Loading

codecov bot commented Mar 26, 2024 •

edited

Loading

IAlibay left a comment •

edited

Loading