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

Conversation

@ShreyasFadnavis
Copy link
Member

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

This comment has been minimized.

Copy link

commented Jul 18, 2019

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

This comment has been minimized.

Copy link

commented Jul 19, 2019

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

This comment has been minimized.

Copy link
Member Author

commented Jul 22, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Jul 28, 2019

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

This comment has been minimized.

Copy link
Member Author

commented Jul 28, 2019

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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Member Author

commented Jul 30, 2019

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 nipy:master Aug 1, 2019

5 checks passed

Codacy/PR Quality Review Up to standards. A positive pull request.
Details
codecov/patch Coverage not affected when comparing d80d01d...d676067
Details
codecov/project 85.43% remains the same compared to d80d01d
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jhlegarreta

This comment has been minimized.

Copy link
Contributor

commented Aug 1, 2019

@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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Member Author

commented Aug 2, 2019

@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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Member Author

commented Aug 2, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.