verigak / progress

Easy to use progress bars for Python
ISC License
1.41k stars 179 forks source link

modified ETA - decreased I/O cost #30

Closed aduriseti closed 8 years ago

aduriseti commented 8 years ago

now uses ciel(num_remaining * global_time_avg_per_item) to estimate ETA

aduriseti commented 8 years ago

Now also prevents I/O cost during progress bar update from occuring more than 1/dt (10 by default) times a second

praiskup commented 8 years ago

Have you tried to look at https://github.com/python-progress/python-progress? That code has been already (a) fixed and (b) optimized.

praiskup commented 8 years ago

Also, have a look at verigak/progress#23.

verigak commented 8 years ago

Please see my inline comments. Also make sure you follow PEP8. I'm getting 6 issues with your PR:

__init__.py:69:5: E303 too many blank lines (2)
__init__.py:70:18: E701 multiple statements on one line (colon)
__init__.py:74:9: E265 block comment should start with '# '
__init__.py:74:80: E501 line too long (103 > 79 characters)
__init__.py:83:5: E303 too many blank lines (2)
__init__.py:99:26: E701 multiple statements on one line (colon)
aduriseti commented 8 years ago

Addressed pep8 flags and inline comments

verigak commented 8 years ago

The thing is that you basically reverted the moving average algorithm to a simple average as a side-effect of your change. I'm open to alternative algorithms, but make it clear what you are doing. For example now _xput is not being used at all, but still stuff are appended to it.

I would rather keep just the update() change and if you'd like to suggest a different algorithm do it in a separate PR, making it clear what the change is all about.

aduriseti commented 8 years ago

Makes sense - I'll fork and make a separate PR.

praiskup commented 8 years ago

@aduriseti have you tried the project I pointed you at? There is O(1) algorithm implemented .. I had similar issue with the code as you (high network speed). Most probably you wouldn't have a problem anymore. But I just want to be sure that I should not fix my fork, too.

praiskup commented 8 years ago

Ah, I now see clearly your issue. This is exactly what I told to @verigak ... in verigak/progress#23 and verigak/progress#24, that there is non-constant algorithm for avg() --> iterating through array for each next() call is wrong.

Just moving update() is not correct, because we don't calculate the last N into average (I've already told to @verigak). And during the first calls to next() this makes progress give you big lie usually, because the speed is getting higher until you reach your bandwidth limit.

Also, you must count with extremely slow connections too ... so there could happen that N==0 is returned and the update() gets very slowly (or never) called. Moving update makes this worse.

I would suggest you implementing the constant algorithm so you don't have to limit the update() calls, or simply have a look at python-progress/python-progress.

aduriseti commented 8 years ago

I have checked it out - I like the time based as opposed to item based window size

albertferras commented 8 years ago

+1 can we merge this?

aduriseti commented 8 years ago

The consensus was that I create separate pull requests for the scheduling and optimization changes. I will do it - but work has been busy recently.