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

[WIP] - MTMS-CSD Tutorial #1909

Merged
merged 1 commit into from Aug 1, 2019
Merged

Conversation

ShreyasFadnavis
Copy link
Member

@ShreyasFadnavis ShreyasFadnavis commented Jul 18, 2019

This tutorial is also intended to delineate DIPY's ability to create pipelines.
Steps involved:

  • Denoising
  • Registration
  • Tissue Classifier
  • MCSD

Addresses #1870

@pep8speaks
Copy link

Hello @ShreyasFadnavis, Thank you for submitting the Pull Request !

Line 44:1: E265 block comment should start with '# '
Line 46:1: E265 block comment should start with '# '
Line 48:1: E265 block comment should start with '# '
Line 67:1: E265 block comment should start with '# '
Line 81:1: E265 block comment should start with '# '
Line 82:1: E265 block comment should start with '# '
Line 85:1: E265 block comment should start with '# '
Line 124:1: E265 block comment should start with '# '
Line 125:1: E265 block comment should start with '# '

Do see the DIPY coding Style guideline

@codecov-io
Copy link

Codecov Report

Merging #1909 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1909   +/-   ##
=======================================
  Coverage   85.43%   85.43%           
=======================================
  Files         119      119           
  Lines       14298    14298           
  Branches     2243     2243           
=======================================
  Hits        12215    12215           
  Misses       1575     1575           
  Partials      508      508

@ShreyasFadnavis
Copy link
Member Author

Hi all! I am facing some issues with the data for this example.. The only viable option that I see from the fetcher is CFIN, but needs some brain extraction on the T1.
I have a working example with the HCP data, but we do not have a fetcher for it! any suggestions would be super helpful..

@jhlegarreta
Copy link
Contributor

The HCP data requires a username/pwd to be able to download it. @arokem mentioned in PR #1900 that the terms and conditions of the HCP data prevent to redistribute them, and this is one of the reasons for having the CENIR dataset in DIPY. CENIR does not have T1-weighted images, though.

I had a quick look at the UW-Minn HCP data terms, and according to point 4, data and derivatives may be re-distributed under the same data use terms. So for that part of the data may be DIPY could comply with those restrictions? May be confirmation should be asked for to UW-Minn HCP team?

If that is feasible and the data of interest for DIPY is part of the UW-Minn HCP data, then may be it could be hosted in UW's digital library, as it is the case of the CENIR data, and the corresponding fetcher could then be built?

@ShreyasFadnavis
Copy link
Member Author

The HCP data requires a username/pwd to be able to download it. @arokem mentioned in PR #1900 that the terms and conditions of the HCP data prevent to redistribute them, and this is one of the reasons for having the CENIR dataset in DIPY. CENIR does not have T1-weighted images, though.

I had a quick look at the UW-Minn HCP data terms, and according to point 4, data and derivatives may be re-distributed under the same data use terms. So for that part of the data may be DIPY could comply with those restrictions? May be confirmation should be asked for to UW-Minn HCP team?

If that is feasible and the data of interest for DIPY is part of the UW-Minn HCP data, then may be it could be hosted in UW's digital library, as it is the case of the CENIR data, and the corresponding fetcher could then be built?

Hi @jhlegarreta ! I completely agree with you. I have reached out to the HCP team and am awaiting a response. In the meantime, let me also take a look at the clauses of UW-Minn HCP ✔

@arokem
Copy link
Contributor

arokem commented Jul 30, 2019

Sorry - I believe that the HCP data is a no go for us. If I understand correctly, you can redistribute the data, but you have to put in place a mechanism for downstream users to also agree to the original terms of conditions, and we can't do that in our software. Please use the CENIR data, or some other data. Both here and in #1900.

@ShreyasFadnavis
Copy link
Member Author

Hi @arokem , the issue is that CENIR does not have a T1. The next option that we have using CFIN, but is not extracted, will need something like BET + registration to do this.. Can do this, but I dont want to go outside DIPY tools for the tutorial. Does this make sense?

@skoudoro skoudoro merged commit d676067 into dipy:master Aug 1, 2019
@jhlegarreta
Copy link
Contributor

@skoudoro Sorry to chime in the middle of the release process, but I am wondering why the file added in this example is not shown in the master branch (either in GitHub or when fetching the branch locally). Am I missing something?

BTW, was the example left out of the examples_index.rst on purpose (e.g. needs polishing)?

@skoudoro
Copy link
Member

skoudoro commented Aug 1, 2019

Strange...

  • First, This tutorial should not be merged it is not ready at all...
  • Second, I do not remember that I merged this PR, it is not even on my mailbox,
  • Third, I do not see it on master too...

Something weird happens.... Github bug? need to investigate....

Thank you for pinging me @jhlegarreta !

@skoudoro
Copy link
Member

skoudoro commented Aug 1, 2019

I really do not know..... Somehow, this PR is linked to #1931 which I merged yesterday. I suppose #1931 was build on the top of this branch but that's still weird, I really do not know what happens here! @arokem @Garyfallidis, @ShreyasFadnavis any idea?

@arokem
Copy link
Contributor

arokem commented Aug 1, 2019

Looks like it wasn't actually merged at all, right? So no big deal (though mysterious...). I think that @ShreyasFadnavis would need to create a new PR. Either from this branch or from a new branch.

@ShreyasFadnavis
Copy link
Member Author

@jhlegarreta Thank your for pointing this out! This is scary! @arokem @skoudoro are right that this was never merged is probably a bug! Will open up a new PR for this. Nevertheless, any idea on what data set to use for this example? This tutorial needs T1 (extracted) + DWI...

@arokem
Copy link
Contributor

arokem commented Aug 2, 2019

Would a "pseudo-T1" work? You can create one from DWI data with an Anisotropic Power Map (see e.g., https://gist.github.com/arokem/5d7441902669a4a82389)

@ShreyasFadnavis
Copy link
Member Author

Thanks @arokem !! Let me this a shot... it should work I guess ✔

@ShreyasFadnavis ShreyasFadnavis deleted the mcsd_tutorial branch September 24, 2019 22:34
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.

None yet

6 participants