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

Including MNI Template 2009c in Fetcher #1106

Merged
merged 7 commits into from Aug 10, 2016

Conversation

Projects
None yet
5 participants
@riddhishb
Contributor

riddhishb commented Aug 2, 2016

This PR readjusts the MNI template data in fetcher.py and also includes MNI Template T1 data 2009c and it's brain mask in the data along with the 2009a T1 and T2 files. The 2009c template is useful in doing template based brain extraction #1096.

The changes made are the following

  • All the flies are stored in figshare
  • A version argument = "a" or "c" in read_mni_template is given for choice of data selection.

@arokem @Garyfallidis Do have a look at this

@coveralls

This comment has been minimized.

coveralls commented Aug 2, 2016

Coverage Status

Coverage decreased (-0.05%) to 82.862% when pulling 9a99a54 on riddhishb:mni_fetcher into 5edee6b on nipy:master.

@codecov-io

This comment has been minimized.

codecov-io commented Aug 2, 2016

Current coverage is 80.78% (diff: 21.42%)

Merging #1106 into master will decrease coverage by 0.05%

@@             master      #1106   diff @@
==========================================
  Files           200        200          
  Lines         23023      23040    +17   
  Methods           0          0          
  Messages          0          0          
  Branches       2309       2318     +9   
==========================================
  Hits          18614      18614          
- Misses         3933       3950    +17   
  Partials        476        476          

Powered by Codecov. Last update 5edee6b...8499605

@coveralls

This comment has been minimized.

coveralls commented Aug 2, 2016

Coverage Status

Coverage decreased (-0.05%) to 82.862% when pulling 558aad6 on riddhishb:mni_fetcher into 5edee6b on nipy:master.

@@ -592,16 +595,19 @@ def read_syn_data():
"""


def read_mni_template(contrast="T2"):
def read_mni_template(version, contrast="T1"):

This comment has been minimized.

@RafaelNH

RafaelNH Aug 9, 2016

Contributor

should we add a default?

folder, 'mni_icbm152_t1_tal_nlin_asym_09c.nii'), "mask": pjoin(
folder, 'mni_icbm152_t1_tal_nlin_asym_09c_mask.nii')}

if contrast == "T2" and version == "c":

This comment has been minimized.

@RafaelNH

RafaelNH Aug 9, 2016

Contributor

You are not covering however the case:
T1_nifti, T2_nifti = read_mni_template("c", contrast = ["T1", "T2"])

if contrast == "mask" and version == "a":
raise ValueError("No template mask available for MNI 2009a")

if not(isinstance(contrast,str)) and version == "c":

This comment has been minimized.

@RafaelNH

RafaelNH Aug 9, 2016

Contributor

PEP8 - missing a space - isinstance(contrast, str)

@coveralls

This comment has been minimized.

coveralls commented Aug 9, 2016

Coverage Status

Coverage decreased (-0.06%) to 82.847% when pulling 8499605 on riddhishb:mni_fetcher into 5edee6b on nipy:master.

@RafaelNH

This comment has been minimized.

Contributor

RafaelNH commented Aug 9, 2016

For my side this is ready to be merged! Does anyone else have any comments?

@coveralls

This comment has been minimized.

coveralls commented Aug 9, 2016

Coverage Status

Coverage decreased (-0.06%) to 82.847% when pulling 8499605 on riddhishb:mni_fetcher into 5edee6b on nipy:master.

@RafaelNH RafaelNH merged commit b64971b into nipy:master Aug 10, 2016

1 of 4 checks passed

codecov/patch 21.42% of diff hit (target 80.84%)
Details
codecov/project 80.78% (-0.06%) compared to 5edee6b
Details
coverage/coveralls Coverage decreased (-0.06%) to 82.847%
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@@ -592,16 +595,19 @@ def read_syn_data():
"""


def read_mni_template(contrast="T2"):
def read_mni_template(version="a", contrast="T1"):

This comment has been minimized.

@jchoude

jchoude Aug 10, 2016

Contributor

I know this PR is already merged, but isn't it a bad idea to silently change the default value of some parameter like this? Someone calling this from a script without any parameter might suddently encounter bizarre results...

This comment has been minimized.

@riddhishb

riddhishb Aug 10, 2016

Contributor

A very valid point, the default value was changed because the version c
does not have a T2 image, so to rectify this we can either rechange the
default to T2 and it will throw a ValueError when version=c.
Or we can also include T2 for version c in the fetcher that way the default
can remain same.

On 10-Aug-2016 7:44 pm, "Jean-Christophe Houde" notifications@github.com
wrote:

In dipy/data/fetcher.py
#1106 (comment):

@@ -592,16 +595,19 @@ def read_syn_data():
"""

-def read_mni_template(contrast="
T2"):
+def read_mni_template(version="a", contrast="T1"):

I know this PR is already merged, but isn't it a bad idea to silently
change the default value of some parameter like this? Someone calling this
from a script without any parameter might suddently encounter bizarre
results...


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/nipy/dipy/pull/1106/files/8499605f20b135f1ae80cf1020bcd75e55eafbd5#r74251872,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AHkvV7xWulDpA1_vJ6ViATXpUBm3-Q23ks5qedyqgaJpZM4JahiD
.

This comment has been minimized.

@jchoude

jchoude Aug 10, 2016

Contributor

Well, I personally think it's better for backwards compatibility to keep the old value.

Also, which template was fetched before the 2 versions were added? Asking because I just want to know if setting version="a" will keep the same behavior as before for default cases.

This comment has been minimized.

@riddhishb

riddhishb Aug 10, 2016

Contributor

@jchoude It was version "a" which was being fetched when there was only a single version, so that would not cause any issues.

This comment has been minimized.

@jchoude

jchoude Aug 10, 2016

Contributor

Good 👍

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