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

Upgrade on DKI implementations #2009

Merged
merged 27 commits into from Jan 3, 2020
Merged

Upgrade on DKI implementations #2009

merged 27 commits into from Jan 3, 2020

Conversation

RafaelNH
Copy link
Contributor

@RafaelNH RafaelNH commented Dec 2, 2019

Hi all! Four years have passed since we've implemented the DKI module, so I've decided to do a review on its code and I've decided to give it an upgrade.

Here are some features added:

  1. Non-linear and Restore DKI fits - Basically I've made the DTI's non-linear and RESTORE fits compatible with DKI, so now dki.py can call these fitting procedures directly from dti.py.

  2. Added some numerical solutions to the estimation of axial, radial and mean kurtosis. Theoretically, these should give the same results than the analytical solutions that we previously implemented. However, numerical solutions can be more stable for some voxels with parameters near to the analytical solution singularities.

  3. Our previous implementations did not have a quantification of kurtosis anisotropy. Therefore, I've decided to add the Kurtosis Fractional Anisotropy index according to Glenn et al. (2015). Also added the mean kurtosis tensor as proposed by Hansen & Jespersen (2013)

  4. New metrics are already added in DKI example.

in future we can incorporte t-designs for different number of directions
evecs for perpendicular direction calculation was not properly selected
this was an issue if data is masked
mean kurtosis tensor has to be equal to standard MK in isotropic diffusion
a bug in parameter indexing for mean kurtosis tensor calculation was fixed
…echniques based on cumulant expansion

other cumulant expansion includes dki and future techniques such as the correlation tensor imaging (CTI)
subfuntions of OLS and WLS fit have to appear first than main functions
minor doc corrections
clip of maximum has to be done after applying normalization factor of 1/5
Copy link
Contributor

@arokem arokem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome stuff. One thing that's not crystal clear to me: what is the advantage of the "numerical" solutions?

dipy/reconst/dki.py Outdated Show resolved Hide resolved
dipy/reconst/dki.py Outdated Show resolved Hide resolved
dipy/reconst/dki.py Outdated Show resolved Hide resolved
dipy/reconst/dki.py Show resolved Hide resolved
dipy/reconst/dki.py Outdated Show resolved Hide resolved
dipy/reconst/dki.py Outdated Show resolved Hide resolved
dipy/reconst/dti.py Outdated Show resolved Hide resolved
dipy/reconst/dti.py Outdated Show resolved Hide resolved
dipy/reconst/dti.py Outdated Show resolved Hide resolved
dipy/reconst/tests/test_dki.py Outdated Show resolved Hide resolved
@RafaelNH
Copy link
Contributor Author

RafaelNH commented Dec 3, 2019

@arokem - answering your question. In theory, numerical and analytical solutions should give the exact same solution; however, numerical solution avoids some instabilities for voxels corresponding to the singularities of the analytical solution.

@RafaelNH
Copy link
Contributor Author

RafaelNH commented Dec 3, 2019

Example: If we run the simulations in MSDKI tutorial, you can see some discontinuities in MK curves for F=0.2. These are caused by instabilities of the estimates near the analytical solution singularities.

Screen Shot 2019-12-03 at 10 37 05

Rerunning the maps using MK´s numerical solution:

Screen Shot 2019-12-03 at 10 36 26

@codecov
Copy link

codecov bot commented Dec 18, 2019

Codecov Report

Merging #2009 into master will increase coverage by 0.62%.
The diff coverage is 92.22%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2009      +/-   ##
==========================================
+ Coverage   90.51%   91.14%   +0.62%     
==========================================
  Files         242      246       +4     
  Lines       31023    31539     +516     
  Branches     3254     3302      +48     
==========================================
+ Hits        28081    28745     +664     
+ Misses       2231     2081     -150     
- Partials      711      713       +2
Impacted Files Coverage Δ
dipy/reconst/tests/test_dki.py 100% <100%> (ø) ⬆️
dipy/data/__init__.py 81.9% <100%> (+0.08%) ⬆️
dipy/reconst/utils.py 100% <100%> (ø) ⬆️
dipy/sims/voxel.py 91.05% <100%> (ø) ⬆️
dipy/reconst/dti.py 94.05% <82.22%> (-0.7%) ⬇️
dipy/reconst/dki.py 96.37% <92.63%> (-0.44%) ⬇️
dipy/workflows/tests/test_reconst_mapmri.py 88.63% <0%> (-4.02%) ⬇️
dipy/segment/bundles.py 89.14% <0%> (-2.23%) ⬇️
dipy/align/streamlinear.py 85.25% <0%> (-1.85%) ⬇️
dipy/workflows/tests/test_masking.py 90% <0%> (-1.67%) ⬇️
... and 58 more

@skoudoro skoudoro modified the milestones: 1.1, 1.2 Dec 23, 2019
@pep8speaks
Copy link

Hello @RafaelNH, Thank you for updating !

Line 175:9: W291 trailing whitespace

@RafaelNH
Copy link
Contributor Author

RafaelNH commented Jan 3, 2020

Just updated the documentation to address issue #2003 .

All done at my side. Let me know if there is something else that you want me to address.

@skoudoro skoudoro mentioned this pull request Jan 3, 2020
16 tasks
@skoudoro skoudoro modified the milestones: 1.2, 1.1 Jan 3, 2020
@arokem
Copy link
Contributor

arokem commented Jan 3, 2020

Looks great! Could you please rebase this one more time? I think that you might have to make some adjustments to the changes you introduced in dipy.data and similarly to the examples to be consistent with changes in #2020 and #2021.

@skoudoro
Copy link
Member

skoudoro commented Jan 3, 2020

I can manage this part and update this example on #2021 as soon as you merge this PR @arokem. No worries 😄

@arokem
Copy link
Contributor

arokem commented Jan 3, 2020

Off we go then!

@arokem arokem merged commit e584392 into dipy:master Jan 3, 2020
@arokem arokem mentioned this pull request Jan 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants