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

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

Merged
merged 2 commits into from
Mar 21, 2016

Conversation

ghoshbishakh
Copy link
Contributor

fixes #970

@arokem
Copy link
Contributor

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
Copy link
Contributor Author

@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
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor

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

…size s

Fixes dipy#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
Copy link
Contributor Author

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

@arokem
Copy link
Contributor

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 mentioned this pull request Mar 16, 2016
arokem added a commit that referenced this pull request Mar 21, 2016
Add progress bar feature to dipy.data.fetcher
@arokem arokem merged commit 209d551 into dipy:master Mar 21, 2016
@arokem
Copy link
Contributor

arokem commented Mar 21, 2016

Closes #970

ghoshbishakh added a commit to ghoshbishakh/dipy that referenced this pull request Mar 29, 2016
Previously file url was used to test the fetcher module.
To improve that and make it compatible with the progressbar in fetcher dipy#971 use a local HTTP server to emulate HTTP downloads.
modified:   tests/test_fetcher.py
quantshah pushed a commit to quantshah/dipy that referenced this pull request Apr 10, 2016
Previously file url was used to test the fetcher module.
To improve that and make it compatible with the progressbar in fetcher dipy#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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Downloading data with dipy.data.fetcher does not show any progress bar
2 participants