tqdm ProgressBar implementation by cbouy · Pull Request #2617 · MDAnalysis/mdanalysis

cbouy · 2020-03-12T14:50:45Z

Changes made in this Pull Request:

Added the ProgressBar class which inherits the tqdm.auto.tqdm object. For now, the disable keyword takes precedence over verbose, i.e. if both are set to True the progress bar won't show. Setting disable=True will disable the progress bar as expected. Setting verbose=False will disable the progress bar.
Deprecate ProgressMeter through warning and text
Replace ProgressMeter with ProgressBar

PR Checklist

Tests?
Docs?
CHANGELOG updated?
Issue raised/referenced?

If run from a jupyter notebook or jupyter lab, will use the tqdm.notebook.tqdm class, else (ipython, shell, or anything else) will use the default tqdm.tqdm class.

cbouy · 2020-03-12T15:50:15Z

Now it should be able to automatically detect Jupyter (notebook or lab) versus the rest and use the appropriate tqdm class.
For JupyterLab, it requires a few things beforehand:

conda install -c conda-forge nodejs
jupyter labextension install @jupyter-widgets/jupyterlab-manager

codecov · 2020-03-12T18:25:52Z

Codecov Report

Merging #2617 into develop will decrease coverage by 0.01%.
The diff coverage is 53.84%.

@@            Coverage Diff             @@
##           develop   #2617      +/-   ##
==========================================
- Coverage    90.71%   90.7%   -0.02%     
==========================================
  Files          174     174              
  Lines        23549   23562      +13     
  Branches      3072    3073       +1     
==========================================
+ Hits         21363   21372       +9     
- Misses        1565    1570       +5     
+ Partials       621     620       -1

Impacted Files	Coverage Δ
package/MDAnalysis/lib/log.py	`92% <53.84%> (-5.71%)`	⬇️
package/MDAnalysis/analysis/hole2/hole.py	`82.71% <0%> (+0.53%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 53ccf6d...6299b90. Read the comment docs.

codecov · 2020-03-12T18:26:01Z

Codecov Report

Merging #2617 into develop will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff            @@
##           develop    #2617   +/-   ##
========================================
  Coverage    91.00%   91.01%           
========================================
  Files          174      174           
  Lines        23550    23595   +45     
  Branches      3083     3082    -1     
========================================
+ Hits         21431    21474   +43     
- Misses        1497     1498    +1     
- Partials       622      623    +1

Impacted Files	Coverage Δ
package/MDAnalysis/analysis/rms.py	`93.54% <ø> (-0.13%)`	⬇️
package/MDAnalysis/analysis/base.py	`100.00% <100.00%> (ø)`
package/MDAnalysis/analysis/density.py	`89.06% <100.00%> (-0.04%)`	⬇️
...ckage/MDAnalysis/analysis/hbonds/hbond_analysis.py	`81.49% <100.00%> (-0.11%)`	⬇️
...age/MDAnalysis/analysis/hbonds/hbond_autocorrel.py	`87.34% <100.00%> (-0.16%)`	⬇️
package/MDAnalysis/analysis/helanal.py	`87.93% <100.00%> (-0.07%)`	⬇️
package/MDAnalysis/analysis/pca.py	`95.29% <100.00%> (-0.27%)`	⬇️
package/MDAnalysis/analysis/waterdynamics.py	`90.51% <100.00%> (-0.14%)`	⬇️
package/MDAnalysis/core/universe.py	`96.33% <100.00%> (-0.03%)`	⬇️
package/MDAnalysis/lib/log.py	`95.78% <100.00%> (-1.92%)`	⬇️
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4102481...ce5051c. Read the comment docs.

orbeckst

I like your new code.

But there's more to be done: The goal of #928 is to replace ProgressMeter with your new ProgressBar everywhere and eventually get rid of our own implementation:

Deprecate ProgressMeter in 1.0 and schedule for removal in 2.0.
- deprecation warning
- deprecation text
Replace all uses of ProgressMeter inside MDAnalysis with equivalent uses of ProgressBar (as much as possible).
Add test for ProgressMeter (testing in ipython/Jupyter is probably not feasible but doing a mock for get_ipython() to show that different backends can be selected should be easy)

.appveyor.yml

package/MDAnalysis/lib/log.py

orbeckst · 2020-03-13T00:14:58Z

@jbarnoud I am adding you to this issue as the original driving force behind #928

…velop

Fixed the empty ProgressBar appearing in notebooks and fixed its total number of iterations displayed

package/MDAnalysis/analysis/base.py

Fixed errors when calculating RMSD Modified the test of the progress bar in TestRMSD Cleanup of useless n_frames and enumerate

orbeckst · 2020-03-14T21:52:39Z

The Python 2.7 version of the test is failing https://travis-ci.com/github/MDAnalysis/mdanalysis/jobs/298190141, perhaps something with comparing utf-8 strings.

jbarnoud

From a very quick look on my phone, the only thing I am missing is a test assessing that the deprecation warning is issued. Good work here 👍

cbouy · 2020-03-15T17:43:53Z

@jbarnoud should I make a proper test module for the ProgressBar and ProgressMeter then ? I find it counter-intuitive to have the test for the ProgressMeter alongside the tests for the RMSD

jbarnoud · 2020-03-15T18:41:21Z

I expect a test that creates a ProgresdMeter and makes sure the deprecation warning is issued. It should probably go with the tests of the log module.The point is to make sure that users that use the progress meter on its own actually see the warning, and that displaying it does not cause an error. We had issues like that in the past, where raising an exception was itself causing an error, because of format errors in the message. On 15 Mar 2020 17:44, Cédric Bouysset <notifications@github.com> wrote: @jbarnoud should I make a proper test module for the ProgressBar and ProgressMeter then ? I find it counter-intuitive to have the test for the ProgressMeter alongside the tests for the RMSD —You are receiving this because you were mentioned.Reply to this email directly, view it on GitHub, or unsubscribe.

…velop

cbouy · 2020-03-16T09:37:37Z

I don't understand the report from codecov, could someone explain what's missing ? I've added a new class, a test for this new class, I moved a test from test_rms.py to test_log.py and created a new test for the deprecation. There should be an increase in coverage or am I getting this wrong ?

orbeckst · 2020-03-20T00:55:41Z

Codecov can be mysterious...

Sorry for the long silence. Will have a look now.

orbeckst

This looks really awesome. I love how much code you deleted! The tests are nice, too.

I have one blocking comment/question. Please have a look. Otherwise this is pretty much ready to go. Nice work!

package/MDAnalysis/analysis/hbonds/hbond_autocorrel.py

orbeckst · 2020-03-20T01:14:06Z

Codecov can be mysterious...

The above was a bit flippant but what I wanted to say is that it's not always clear that codecov is showing us what we expect to see and although we like to see everything green in the CI, we do overrule Codecov once we are happy.

jbarnoud · 2020-03-20T11:30:58Z

@lilyminium noticed in #2631 (comment) that the API changes. It probably should not.

lilyminium · 2020-03-20T11:40:09Z

#2631 (comment) was phrased poorly -- as far as I can tell this does not change the API, except that AnalysisBase._pm no longer exists. #2631 did alter the arguments given to _setup_frames. This PR also fixes #2504.

orbeckst

Minor CHANGELOG comment and then I will approve and merge.

package/CHANGELOG

orbeckst · 2020-03-20T22:28:44Z

@lilyminium @jbarnoud thanks for pointing out that this PR fixes #2504, too #2631 (comment).

cbouy · 2020-03-21T16:13:39Z

Thank you @orbeckst @jbarnoud @richardjgowers for your help ! And thanks @lilyminium for mentioning the other issue !

orbeckst

Awesome.

Just waiting for CI to come back green to do the squash&merge.

orbeckst · 2020-03-21T20:33:59Z

One of the macOS jobs failed right at the start. I restarted https://travis-ci.com/github/MDAnalysis/mdanalysis/jobs/300680256 – hopefully this will all look good in an hour.

orbeckst · 2020-03-22T17:45:00Z

@cbouy very nice contribution!

Correction for PR MDAnalysis#2256

cbouy added 2 commits March 12, 2020 15:00

tqdm ProgressBar implementation

bb1a6dd

Automatically detect which version of tqdm to use

4c280a9

If run from a jupyter notebook or jupyter lab, will use the tqdm.notebook.tqdm class, else (ipython, shell, or anything else) will use the default tqdm.tqdm class.

cbouy added 2 commits March 12, 2020 17:22

fixed dependencies missing for CI

e0603b8

Merge branch 'develop' into develop

6299b90

orbeckst added the GSOC Starter label Mar 12, 2020

orbeckst requested changes Mar 13, 2020

View reviewed changes

.appveyor.yml Show resolved Hide resolved

.appveyor.yml Outdated Show resolved Hide resolved

package/MDAnalysis/lib/log.py Outdated Show resolved Hide resolved

package/MDAnalysis/lib/log.py Outdated Show resolved Hide resolved

package/MDAnalysis/lib/log.py Outdated Show resolved Hide resolved

orbeckst assigned orbeckst and jbarnoud Mar 13, 2020

cbouy added 5 commits March 13, 2020 12:23

tqdm as conda dependency instead of pip (appveyor)

a46748e

deprecation of ProgressMeter

01cdacc

replaced ProgressMeter by ProgressBar

9dfb879

Merge branch 'develop' of https://github.com/cbouy/mdanalysis into de…

6d5ee8e

…velop

fixed progressbar

c2ef8a7

Fixed the empty ProgressBar appearing in notebooks and fixed its total number of iterations displayed

richardjgowers reviewed Mar 14, 2020

View reviewed changes

package/MDAnalysis/analysis/base.py Outdated Show resolved Hide resolved

progressbar fixes and cleanup

9d561bd

Fixed errors when calculating RMSD Modified the test of the progress bar in TestRMSD Cleanup of useless n_frames and enumerate

Cédric Bouysset and others added 3 commits March 14, 2020 22:59

Converted expected string to unicode

f7f98ce

updated changelog

76a79a3

Merge branch 'develop' into develop

e7aba02

cbouy requested a review from orbeckst March 15, 2020 15:05

jbarnoud reviewed Mar 15, 2020

View reviewed changes

cbouy and others added 3 commits March 15, 2020 19:45

Tests for the log module

88ebc3c

Merge branch 'develop' of https://github.com/cbouy/mdanalysis into de…

cb095fe

…velop

removed unreliable time estimation from the test

b97176e

cbouy requested review from jbarnoud and richardjgowers March 18, 2020 08:14

Merge branch 'develop' into develop

a0a0c09

orbeckst requested changes Mar 20, 2020

View reviewed changes

package/MDAnalysis/analysis/hbonds/hbond_autocorrel.py Outdated Show resolved Hide resolved

package/MDAnalysis/analysis/hbonds/hbond_autocorrel.py Outdated Show resolved Hide resolved

lilyminium mentioned this pull request Mar 20, 2020

Setting Verbose #2631

Closed

4 tasks

cbouy added 2 commits March 20, 2020 14:25

fix wrong total given to ProgressBar

900f871

Merge branch 'develop' into develop

7e881ad

orbeckst requested changes Mar 20, 2020

View reviewed changes

package/CHANGELOG Show resolved Hide resolved

more tests for issue #928 and #2504

ce5051c

orbeckst approved these changes Mar 21, 2020

View reviewed changes

orbeckst merged commit 5f51383 into MDAnalysis:develop Mar 22, 2020

orbeckst mentioned this pull request Apr 6, 2020

Consistent Autocorrelation and Intermittency - Across Different Analysis #2256

Merged

4 tasks

bieniekmateusz added a commit to bieniekmateusz/mdanalysis that referenced this pull request Apr 6, 2020

Inserted back the accidentally removed PR MDAnalysis#2617

0fa3c2b

bieniekmateusz added a commit to bieniekmateusz/mdanalysis that referenced this pull request Apr 6, 2020

Inserted back the accidentally removed PR MDAnalysis#2617

30cf75a

Correction for PR MDAnalysis#2256

mquevill mentioned this pull request Jun 23, 2020

ProgressMeter doesn't work in jupyter lab #2078

Closed

IAlibay mentioned this pull request Jun 23, 2020

Document JupyterLab Progress Bar requirements MDAnalysis/UserGuide#78

Closed

fiona-naughton added enhancement Component-Analysis labels Sep 26, 2023

Conversation

cbouy commented Mar 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Checklist

Uh oh!

cbouy commented Mar 12, 2020

Uh oh!

codecov bot commented Mar 12, 2020

Codecov Report

Uh oh!

codecov bot commented Mar 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

orbeckst left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

orbeckst commented Mar 13, 2020

Uh oh!

Uh oh!

orbeckst commented Mar 14, 2020

Uh oh!

jbarnoud left a comment

Choose a reason for hiding this comment

Uh oh!

cbouy commented Mar 15, 2020

Uh oh!

jbarnoud commented Mar 15, 2020 via email

Uh oh!

cbouy commented Mar 16, 2020

Uh oh!

orbeckst commented Mar 20, 2020

Uh oh!

orbeckst left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

orbeckst commented Mar 20, 2020

Uh oh!

jbarnoud commented Mar 20, 2020

Uh oh!

lilyminium commented Mar 20, 2020

Uh oh!

orbeckst left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

orbeckst commented Mar 20, 2020

Uh oh!

cbouy commented Mar 21, 2020

Uh oh!

orbeckst left a comment

Choose a reason for hiding this comment

Uh oh!

orbeckst commented Mar 21, 2020

Uh oh!

orbeckst commented Mar 22, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

cbouy commented Mar 12, 2020 •

edited

Loading

codecov bot commented Mar 12, 2020 •

edited

Loading