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

Use fastprogress instead of tqdm progressbar #3693

Merged
merged 10 commits into from
Dec 9, 2019

Conversation

aloctavodia
Copy link
Member

Fix problems with #3667

@aloctavodia
Copy link
Member Author

aloctavodia commented Nov 25, 2019

oops I just realized, fastprogressbar requires at least Python 3.6 . It seems that the next numpy release will requiere python 3.6+, thus maybe is ok for us to also require Python 3.6+ in the near future. I guess we should do a a release before dropping 3.5

@aloctavodia aloctavodia mentioned this pull request Dec 1, 2019
@codecov
Copy link

codecov bot commented Dec 8, 2019

Codecov Report

Merging #3693 into master will increase coverage by 27.47%.
The diff coverage is 81.52%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master    #3693       +/-   ##
===========================================
+ Coverage   62.46%   89.94%   +27.47%     
===========================================
  Files         134      134               
  Lines       20194    20413      +219     
===========================================
+ Hits        12615    18361     +5746     
+ Misses       7579     2052     -5527
Impacted Files Coverage Δ
pymc3/sampling.py 82.88% <100%> (+16.3%) ⬆️
pymc3/smc/smc.py 92.6% <70%> (ø) ⬆️
pymc3/variational/inference.py 79.65% <76.59%> (+0.1%) ⬆️
pymc3/parallel_sampling.py 86.61% <81.81%> (+10.23%) ⬆️
pymc3/tuning/starting.py 79.16% <90.9%> (+11.13%) ⬆️
pymc3/distributions/__init__.py 100% <0%> (ø) ⬆️
pymc3/distributions/distribution.py 94.95% <0%> (+0.51%) ⬆️
pymc3/step_methods/slicer.py 95.45% <0%> (+1.51%) ⬆️
pymc3/distributions/discrete.py 74.51% <0%> (+1.93%) ⬆️
pymc3/step_methods/hmc/nuts.py 99.35% <0%> (+4.48%) ⬆️
... and 74 more

@fonnesbeck
Copy link
Member

fonnesbeck commented Dec 9, 2019

Need to add fastprogress to requirements-dev.txt as well.

EDIT: never mind, need to repair our requirements files to make them complementary.

@fonnesbeck
Copy link
Member

LGTM

@@ -346,8 +348,7 @@ def __init__(
start_chain_num=0,
progressbar=True,
):
if progressbar:
from tqdm import tqdm
from fastprogress import progress_bar
Copy link
Member

Choose a reason for hiding this comment

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

i think this can go to the top of the file. if i recall, it was here and protected because of install difficulties with tqdm.

If it stays here, I would leave it in a if progressbar: statement.

desc=self._desc.format(self)
)
self._progress = progress_bar(
range(chains * (draws + tune)), display=progressbar, auto_update=False
Copy link
Member

Choose a reason for hiding this comment

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

oooh, this display=progressbar is fancy! I would move the import to the top for sure, then!

'or Inference instance' %
set(_select.keys()))
raise KeyError(
"method should be one of %s " "or Inference instance" % set(_select.keys())
Copy link
Member

Choose a reason for hiding this comment

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

black did a bad job folding this, I think

Suggested change
"method should be one of %s " "or Inference instance" % set(_select.keys())
"method should be one of %s or Inference instance" % set(_select.keys())

raise TypeError('method should be one of %s '
'or Inference instance' %
set(_select.keys()))
raise TypeError("method should be one of %s " "or Inference instance" % set(_select.keys()))
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
raise TypeError("method should be one of %s " "or Inference instance" % set(_select.keys()))
raise TypeError("method should be one of %s or Inference instance" % set(_select.keys()))

@@ -791,10 +807,12 @@ def fit(
inference = _select[method](model=model, **inf_kwargs)
else:
raise KeyError(
"method should be one of %s " "or Inference instance" % set(_select.keys())
f"method should be one of {set(_select.keys())} or Inference instance"
Copy link
Member

Choose a reason for hiding this comment

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

💯

@ColCarroll ColCarroll merged commit 1c30a6f into pymc-devs:master Dec 9, 2019
@aloctavodia aloctavodia deleted the fastprogress branch December 10, 2019 13:16
@aakhmetz
Copy link

aakhmetz commented Dec 10, 2019

It would be nice to see #iterations per second (currently there is only information about logp (I checked it with find_MAP)). In this sense previous progressbar was more informative

Also it does not show the expected time, just 0:00<0:00

@fonnesbeck
Copy link
Member

@aakhmetz I agree, fastprogress is a simpler progress bar, but I have found it to be more robust than tqdm, which was my original motivation for making the switch.

@aakhmetz
Copy link

I have an ongoing project where I fit a large system of ODEs to the data and where it is desirable to see the current speed for performing iterations.

Please excuse me for a small objection, you definitely foresee much further than me

@fonnesbeck
Copy link
Member

Fair point. You might have a peek at the fastprogress project and either make a feature request or PR. The code for updating the progress bar is pretty simple, so it should be easy to add.

sthagen added a commit to sthagen/pymc-devs-pymc that referenced this pull request Dec 10, 2019
Use fastprogress instead of tqdm progressbar (pymc-devs#3693)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants