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

Spherical deconvolution model CANNOT be constructed without specifying a response #1916

Merged
merged 5 commits into from Jul 23, 2019

Conversation

@RafaelNH
Copy link
Contributor

commented Jul 21, 2019

I've created this PR basically to address issue #283.

After analysing the issue, I've decided that the best solution is to remove the response=None option. If one wants to use a predefined response function, it can just defined it on its own risk by calling:
response=(np.array([AD, RD, RD]), S0)

Using an None option is not ideal also because it does not allows the adjustment of S0. At the current version of the CSD implementation, data is normalised based on this inputed S0 estimate, so having it properly estimated might be crucial for the estimation of the fODF shape.

After revising the CSD example, I've noticed that it might be hard to ready for non diffusion MRI experts. For instance, it might not be clear that the time consuming response function calibration procedure is optional. To make the example easier to read, I've added some sub-headings. Please let me know your comments and suggestions on this issue.

@pep8speaks

This comment has been minimized.

Copy link

commented Jul 21, 2019

Hello @RafaelNH, Thank you for updating !

Line 192:76: W291 trailing whitespace

Comment last updated at 2019-07-21 17:38:08 UTC
RafaelNH added 4 commits Jul 17, 2019
RF: Removing a 'None' response function
The response function should be specified because current version of code assumes that S0 is properly estimated. Therefore a response='None' option should not exist
@skoudoro

This comment has been minimized.

Copy link
Member

commented Jul 21, 2019

Thank you @RafaelNH for fixing this old issue!

Can you have a look at this one @Garyfallidis?

@RafaelNH RafaelNH force-pushed the RafaelNH:CSD_doc branch from be8de77 to 533ce29 Jul 21, 2019

@codecov-io

This comment has been minimized.

Copy link

commented Jul 21, 2019

Codecov Report

Merging #1916 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1916      +/-   ##
==========================================
- Coverage   85.43%   85.42%   -0.01%     
==========================================
  Files         119      119              
  Lines       14298    14296       -2     
  Branches     2243     2242       -1     
==========================================
- Hits        12215    12213       -2     
  Misses       1575     1575              
  Partials      508      508
Impacted Files Coverage Δ
dipy/reconst/csdeconv.py 89.86% <100%> (-0.07%) ⬇️
@Garyfallidis

This comment has been minimized.

Copy link
Member

commented Jul 23, 2019

Thanks @RafaelNH

@Garyfallidis Garyfallidis merged commit 51b6422 into nipy:master Jul 23, 2019

5 checks passed

Codacy/PR Quality Review Up to standards. A positive pull request.
Details
codecov/patch 100% of diff hit (target 85.43%)
Details
codecov/project Absolute coverage decreased by -<.01% but relative coverage increased by +14.56% compared to 47b56a7
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.