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

Add progress bar feature to dipy.data.fetcher #971

Merged
merged 2 commits into from Mar 21, 2016

Conversation

Projects
None yet
2 participants
@ghoshbishakh
Member

ghoshbishakh commented Mar 15, 2016

fixes #970

@arokem

This comment has been minimized.

Member

arokem commented Mar 15, 2016

Nice one. +1 from me.

Not to bike-shed too much, but it would be nice to have the eventual file-size reported as part of the progress bar (we currently report file size "by hand"). That way, users know what "10%" of the file-size means.

@ghoshbishakh

This comment has been minimized.

Member

ghoshbishakh commented Mar 15, 2016

@arokem sure so should I remove
_log('Data size is approximately %s' % data_size) ?

eventual file-size reported as part of the progress bar

by as a part of progress bar you mean beside it? or above it?

@arokem

This comment has been minimized.

Member

arokem commented Mar 15, 2016

Yes - if we can figure out data_size from the file (apparently you can!), we can get rid of all the places where data_size is specified by hand, and replace it by the report alongside the progress bar.

Maybe put the size of the file right next to the percentage already downloaded?

@ghoshbishakh

This comment has been minimized.

Member

ghoshbishakh commented Mar 15, 2016

screenshot from 2016-03-15 20-58-49
Things are looking a bit cluttered I guess? @arokem
Should I remove the "Download Progress:" string?

@arokem

This comment has been minimized.

Member

arokem commented Mar 15, 2016

This actually looks fine to me.

On Tue, Mar 15, 2016 at 8:29 AM, Bishakh Ghosh notifications@github.com
wrote:

[image: screenshot from 2016-03-15 20-58-49]
https://cloud.githubusercontent.com/assets/5156787/13783279/c5416116-eaf0-11e5-88e3-40089964468e.png
Things are looking a bit cluttered I guess? @arokem
https://github.com/arokem
Should I remove the "Download Progress:" string?


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
#971 (comment)

@arokem

This comment has been minimized.

Member

arokem commented Mar 15, 2016

We'd want to get rid of all the other places that have the data_size.

In the definition of fetch_data, and make_fetcher:

https://github.com/nipy/dipy/blob/master/dipy/data/fetcher.py#L88
https://github.com/nipy/dipy/blob/master/dipy/data/fetcher.py#L133

But also in the definition of individual fetchers. For example:
https://github.com/nipy/dipy/blob/master/dipy/data/fetcher.py#L284

Display data_size beside the progress bar and remove hard coded data_…
…size s

Fixes #970
Data size was printed by hand previously i.e hard coded. This is replacing that with the data size from the http response and displaying beside the progressbar.
@ghoshbishakh

This comment has been minimized.

Member

ghoshbishakh commented Mar 16, 2016

@arokem Made the changes! I hope that does not break anything anywhere else.

@arokem

This comment has been minimized.

Member

arokem commented Mar 16, 2016

Yes. That looks right to me. If anyone else wants to take a look, please do so in the next few days. Otherwise, I will go ahead and merge this.

@arokem arokem referenced this pull request Mar 16, 2016

Merged

Fix PEP8 in data #929

arokem added a commit that referenced this pull request Mar 21, 2016

Merge pull request #971 from ghoshbishakh/docsPy3
Add progress bar feature to dipy.data.fetcher

@arokem arokem merged commit 209d551 into nipy:master Mar 21, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@arokem

This comment has been minimized.

Member

arokem commented Mar 21, 2016

Closes #970

ghoshbishakh added a commit to ghoshbishakh/dipy that referenced this pull request Mar 29, 2016

Fix fetcher tests
Previously file url was used to test the fetcher module.
To improve that and make it compatible with the progressbar in fetcher nipy#971 use a local HTTP server to emulate HTTP downloads.
modified:   tests/test_fetcher.py

sahmed95 pushed a commit to sahmed95/dipy that referenced this pull request Apr 10, 2016

Fix fetcher tests
Previously file url was used to test the fetcher module.
To improve that and make it compatible with the progressbar in fetcher nipy#971 use a local HTTP server to emulate HTTP downloads.
modified:   tests/test_fetcher.py
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment