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

Allow hiding progressbar until some time interval has elapsed. #836

Merged
merged 3 commits into from Feb 25, 2021

Conversation

anntzer
Copy link
Contributor

@anntzer anntzer commented Nov 7, 2019

This avoids printing the progressbar for very short loops, i.e. the feature requested in #704. I am not particularly wedded to the kwarg name, alternatives are welcome.

@casperdcl casperdcl self-assigned this Nov 7, 2019
@casperdcl casperdcl added p4-enhancement-future 🧨 On the back burner to-review 🔍 Awaiting final confirmation labels Nov 7, 2019
@codecov-io
Copy link

codecov-io commented Nov 12, 2019

Codecov Report

Merging #836 (90cb341) into devel (d710226) will decrease coverage by 0.05%.
The diff coverage is 83.33%.

@@            Coverage Diff             @@
##            devel     #836      +/-   ##
==========================================
- Coverage   84.81%   84.75%   -0.06%     
==========================================
  Files          24       24              
  Lines        1613     1620       +7     
  Branches      265      267       +2     
==========================================
+ Hits         1368     1373       +5     
- Misses        208      209       +1     
- Partials       37       38       +1     

@anntzer
Copy link
Contributor Author

anntzer commented Feb 19, 2020

Kindly bumping, thanks!

@casperdcl casperdcl added this to the v5.0.0 milestone Feb 19, 2020
@casperdcl casperdcl added p3-enhancement 🔥 Much new such feature to-merge ↰ Imminent and removed p4-enhancement-future 🧨 On the back burner to-review 🔍 Awaiting final confirmation labels Feb 19, 2020
Copy link
Sponsor Member

@casperdcl casperdcl left a comment

Choose a reason for hiding this comment

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

Nice but maybe use an existing kwarg?

tqdm/std.py Outdated
@@ -775,6 +775,7 @@ def __init__(self, iterable=None, desc=None, total=None, leave=True,
unit_scale=False, dynamic_ncols=False, smoothing=0.3,
bar_format=None, initial=0, position=None, postfix=None,
unit_divisor=1000, write_bytes=None, lock_args=None,
waituntil=0,
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

@anntzer how about using disable=<float> instead?

We could do:

if disable and disable not in (True, 1):
    disable = False
    waituntil = disable

This would be backward-compatible without adding yet another kwarg (similar to unit_scale):

if unit_scale and unit_scale not in (True, 1):

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not clear to me these can be combined -- what if I want to use waituntil=1 for example?

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Yes as with unit_scale it would be treated the same as True. You'd need 0.99 or 1.01 etc if you really wanted approx 1 second

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But then I fail to see why you want to combine these into a single kwarg (I'm all for not adding more kwargs than necessary, but not at the cost of combining unrelated stuff)... disable and waituntil have completely different semantics, and the combination you propose just looks... confusing?
Another way to look at it is, what docstring do you propose?

disable : bool or float, optional

If True, the entire progressbar wrapper is disabled.
If None (the default), the progressbar wrapper is disabled on non-TTY.
If a float value other than 1, the progressbar is not displayed until disable seconds have elapsed

?

Copy link
Sponsor Member

@casperdcl casperdcl Mar 25, 2020

Choose a reason for hiding this comment

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

Something like:

    disable  : bool or float, optional
        [default: False].
        If True or 1, disable the entire progressbar wrapper.
        If None, disabled on non-TTY.
        If any other numeric value, wait until the specified number
        of seconds have elapsed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me rephrase my query, then: Are willing to merge (or at least review) a PR which introduces the feature under the disable kwarg? If so, then I am willing to ignore my opinion that folding the feature into disable is a mistake.

Copy link
Sponsor Member

@casperdcl casperdcl Mar 25, 2020

Choose a reason for hiding this comment

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

I think we're willing to merge this feature, and there are 2 ways to expose it to an end user (new kwarg or augment existing kwarg). Both are easy to implement, but I don't know what users will find best. More opinions from other people would help.

advantages:

  • new kwarg
    • no risk of breaking backward compatibility
    • supports exactly 1 second
  • augment disable
    • no additional kwarg clutter
    • no need to decide on new kwarg name
    • consistency: same implementation approach as unit_scale

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I'll let you figure this out then :) (I have already given my arguments above, and don't have much to add.)

Copy link

Choose a reason for hiding this comment

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

I'm definitely pro new kwarg. The alternative is WAY confusing. It would make it basically impossible to find the feature at all. If one does, one can use any amount of seconds except 1? Really?

Using unit_scale for two things is less bad because nobody wants an actual unit_scale of 1.

Copy link

Choose a reason for hiding this comment

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

@casperdcl I agree with @graue70 and @anntzer. Folding waituntil into disable seems hostile to the user w.r.t. discoverability and the unintuitive casting of 1.0 to a boolean. As for an alternative name, how about delay?

@casperdcl
Copy link
Sponsor Member

casperdcl commented Nov 7, 2020

fixes #1069
fixes #704

@casperdcl casperdcl added this to In Progress in Casper Nov 7, 2020
@casperdcl casperdcl added the need-feedback 📢 We need your response (question) label Nov 7, 2020
@casperdcl casperdcl linked an issue Nov 7, 2020 that may be closed by this pull request
@casperdcl casperdcl changed the base branch from master to devel February 25, 2021 16:09
@casperdcl casperdcl added c1-quick 🕐 Complexity low and removed need-feedback 📢 We need your response (question) labels Feb 25, 2021
@casperdcl casperdcl moved this from In Progress to Next Release in Casper Feb 25, 2021
@casperdcl casperdcl mentioned this pull request Feb 25, 2021
@casperdcl casperdcl merged commit c76af4d into tqdm:devel Feb 25, 2021
Casper automation moved this from Next Release to Done Feb 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c1-quick 🕐 Complexity low p3-enhancement 🔥 Much new such feature to-merge ↰ Imminent
Projects
Casper
  
Done
Development

Successfully merging this pull request may close these issues.

Proposal: option for making tqdm quiet until some time has passed
5 participants