Skip to content

Fix maxinterval to adjust miniters to mininterval + fix dynamic_miniters when smoothing #250

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
Merged
merged 4 commits into from
Nov 12, 2016
Merged

Fix maxinterval to adjust miniters to mininterval + fix dynamic_miniters when smoothing #250

merged 4 commits into from
Nov 12, 2016

Conversation

lrq3000
Copy link
Member

@lrq3000 lrq3000 commented Aug 28, 2016 •
edited
Loading

... + Deprecation warning test to get 100% coverage again + Update readme and docstrings to provide more details about these parameters.

Fix regressions spotted in #249 (maxinterval fixing miniters too high) and in #258 (smoothing=0 breaks dynamic_miniters, also old calculation made tqdm faster, so it was reinstated).

Maxinterval would sometimes fix miniters to a value higher than it should, because it was adjusting miniters according to maxinterval instead of mininterval (the other conditions for dynamic_miniters are ok and adjusted better than maxinterval).

Also updated the readme and docstring to give more details on the various interval parameters (mininterval, maxinterval, miniters). They are critical for lots of people and define most of the somewhat weird behaviors of tqdm, so I think they ought to have extensive documentation both in the readme and the docstring.

TODO:

  • Fix miniters calculation by maxinterval
  • Fix unit test
  • Flake8
  • Add coverage unit test for deprecation exception (100% unit test again for tqdm core)

@lrq3000 lrq3000 added the p0-bug-critical ☢ Exception rasing label Aug 28, 2016
@coveralls
Copy link

coveralls commented Aug 28, 2016 •
edited
Loading

Coverage Status

Coverage decreased (-0.3%) to 90.437% when pulling 295a2a2 on maxinterval-fix into 4b3d916 on master.

@codecov-io
Copy link

codecov-io commented Aug 28, 2016 •
edited
Loading

Current coverage is 91.86% (diff: 100%)

Merging #250 into master will increase coverage by 0.09%

@@             master       #250   diff @@
==========================================
  Files             7          7          
  Lines           535        541     +6   
  Methods           0          0          
  Messages          0          0          
  Branches         97         99     +2   
==========================================
+ Hits            491        497     +6   
  Misses           43         43          
  Partials          1          1

Powered by Codecov. Last update 86292e5...a3bcc8a

@lrq3000 lrq3000 mentioned this pull request Aug 28, 2016
Closed
@lrq3000 lrq3000 added the to-review 🔍 Awaiting final confirmation label Sep 2, 2016
@coveralls
Copy link

coveralls commented Sep 2, 2016 •
edited
Loading

Coverage Status

Coverage increased (+0.3%) to 91.079% when pulling 5fa3f40 on maxinterval-fix into 4b3d916 on master.

@lrq3000 lrq3000 mentioned this pull request Sep 5, 2016
Open
6 tasks
@lrq3000 lrq3000 changed the title Fix maxinterval to adjust miniters to mininterval + Update readme and docstring Fix maxinterval to adjust miniters to mininterval + fix dynamic_miniters when smoothing Sep 5, 2016
@coveralls
Copy link

coveralls commented Sep 5, 2016 •
edited
Loading

Coverage Status

Coverage increased (+0.3%) to 91.079% when pulling 80f1086 on maxinterval-fix into 4b3d916 on master.

@lrq3000 lrq3000 mentioned this pull request Sep 6, 2016
Closed
4 tasks
@casperdcl
Copy link
Member

@lrq3000 how does this fit in with #251 ? Can you update 251 to include whatever additional changes that may be in this PR?

@lrq3000
Copy link
Member Author

lrq3000 commented Oct 29, 2016

@casperdcl There is no interaction between both, the maxinterval fix is related to its interaction with mininterval, not with the monitor thread.

I will update this PR here to fit with the monitor thread.

@lrq3000 lrq3000 mentioned this pull request Oct 29, 2016
Closed
@lrq3000
Copy link
Member Author

lrq3000 commented Oct 30, 2016

Ok I'm done with this PR :)

@casperdcl casperdcl mentioned this pull request Oct 30, 2016
Merged
8 tasks
@lrq3000 lrq3000 mentioned this pull request Oct 30, 2016
Merged
@casperdcl casperdcl force-pushed the master branch 5 times, most recently from 8cade97 to a65e347 Compare October 31, 2016 02:34
@casperdcl casperdcl force-pushed the maxinterval-fix branch 2 times, most recently from 515c660 to 11ffd3e Compare October 31, 2016 15:42
@lrq3000
Copy link
Member Author

lrq3000 commented Oct 31, 2016

Also you noticed that in the docstring and readme, it was still specifying this about miniters:

If specified, will set mininterval to 0.

Which is false since a long time. So I deleted this line.

@lrq3000
Copy link
Member Author

lrq3000 commented Oct 31, 2016

Rebased. Didn't take the time to do the asv test yet.

@lrq3000
Copy link
Member Author

lrq3000 commented Nov 1, 2016 •
edited
Loading

Cool @casperdcl , I saw you made a fast version python setup.py make testasv for quick testing, good idea. I get results that are agreeing with my previous homemade benchmarking, but you should benchmark it yourself because I don't trust my machine's stability currently.

/EDIT: I think on only one commit it's hard to detect a regression using asv...

casperdcl
casperdcl approved these changes Nov 12, 2016
View reviewed changes
Copy link
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.

thanks. slight performance hit is just about measurable but we can live with it

@casperdcl casperdcl merged commit a3bcc8a into master Nov 12, 2016
@casperdcl casperdcl deleted the maxinterval-fix branch November 12, 2016 14:42
@lrq3000
Copy link
Member Author

lrq3000 commented Nov 12, 2016

Ah really? I expected no perf hit since it's only two new conditinals (with one being never evaluated except if smoothing=0).
Maybe it's because i merged 2 dynamic_miniters calculations in 1? I should try to separate them to see if it fixes the perf hit.

@casperdcl
Copy link
Member

yes, you're right. no perf hit. was looking at the wrong thing

@casperdcl casperdcl added p2-bug-warning ⚠ Visual output bad and removed p0-bug-critical ☢ Exception rasing labels Dec 6, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@casperdcl casperdcl casperdcl approved these changes

Assignees

@casperdcl casperdcl

Labels
p2-bug-warning ⚠ Visual output bad to-review 🔍 Awaiting final confirmation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@lrq3000 @coveralls @codecov-io @casperdcl