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

CENIR 'HCP-like' multi b-value data #691

Merged
merged 5 commits into from Aug 3, 2015

Conversation

Projects
None yet
3 participants
@arokem
Member

arokem commented Jul 29, 2015

Needed for #677

@MrBago

This comment has been minimized.

Contributor

MrBago commented Jul 29, 2015

@arokem could you use the fetch_data method in your fetchers? The way fetch_data is written, if the data exists, it confirms the md5 and returns. If the md5 doesn't match or the file does not exist it will download the file. fetch_data has tests written for it, but we cannot write tests for each individual "fetcher."

Also, I think the "read_data" functions should call the "fetch_data" functions. If the files exist, calling the "fetcher" will simply serve to confirm the md5 is correct and the data has not been changed or corrupted. If the file does not exist, then the "fetcher" will try and download the data. Also if the reader is calling the fetcher, the fetcher can return the filenames to the reader so that we don't need to hard-code the file names in 2 different places.

This model makes for less code duplication, better testing and less hard-coding of file names, directories. Look at fetch_standford_labels/read_stanford_labels if you need an example.

@MrBago

This comment has been minimized.

Contributor

MrBago commented Jul 29, 2015

I think we should at some point apply this change to all the "fetchers", but for now I would settle for using it in new "fetchers".

@arokem

This comment has been minimized.

Member

arokem commented Jul 29, 2015

Yeah - good call. I did this on #681 as well.

Once these two are merged (hint, hint), I volunteer to do a full refactor
in this direction on all of the fetchers/readers. This module is indeed a
bit of a mess right now.

On Wed, Jul 29, 2015 at 2:33 PM, Bago Amirbekian notifications@github.com
wrote:

I think we should at some point apply this change to all the "fetchers",
but for now I would settle for using it in new "fetchers".


Reply to this email directly or view it on GitHub
#691 (comment).

@arokem arokem referenced this pull request Jul 29, 2015

Closed

Refactor fetcher.py #692

@arokem arokem force-pushed the arokem:multib-data branch from 69c7e34 to a15975b Jul 29, 2015

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Aug 2, 2015

@MrBago this looks good to me. If you agree then please go ahead and merge this.

@arokem arokem force-pushed the arokem:multib-data branch from a15975b to e410151 Aug 3, 2015

@arokem

This comment has been minimized.

Member

arokem commented Aug 3, 2015

Hang on with this one. As pointed out by @RafaelNH, there's currently no good way to pull the affine out from this. Working on it.

@arokem

This comment has been minimized.

Member

arokem commented Aug 3, 2015

OK - all set with that now.

MrBago added a commit that referenced this pull request Aug 3, 2015

Merge pull request #691 from arokem/multib-data
CENIR 'HCP-like' multi b-value data

@MrBago MrBago merged commit 9b2c19d into nipy:master Aug 3, 2015

1 check passed

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

@arokem arokem referenced this pull request Aug 31, 2015

Merged

Fvtk 2.0 PR1 #587

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment