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

DKI Microstructural model #1226

Merged
merged 63 commits into from Apr 19, 2017

Conversation

Projects
None yet
4 participants
@RafaelNH
Contributor

RafaelNH commented Apr 18, 2017

Hello! This supersedes PR #1027! The history of PR #1027 got a bit messed up after rebasing it with the master and merging some changes that @arokem suggested - sorry for the trouble caused!

This PR contains the same code suggestions that PR #1027 however with the correct development history!

I think I've already covered all the comment of PR #1027 but let me know if you noticed that something is missing. The example of usage still have to be updated, but I will start this fresh using the multi-shell data that @arokem added in PR #1225.

@RafaelNH RafaelNH requested a review from arokem Apr 18, 2017

@RafaelNH

This comment has been minimized.

Contributor

RafaelNH commented Apr 18, 2017

Example of usage done! :)

@coveralls

This comment has been minimized.

coveralls commented Apr 18, 2017

Coverage Status

Coverage increased (+0.2%) to 88.563% when pulling e2d2436 on RafaelNH:DKI_microstructure_history into 568d14c on nipy:master.

@codecov-io

This comment has been minimized.

codecov-io commented Apr 18, 2017

Codecov Report

Merging #1226 into master will increase coverage by 0.18%.
The diff coverage is 96.74%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1226      +/-   ##
==========================================
+ Coverage   85.88%   86.07%   +0.18%     
==========================================
  Files         221      223       +2     
  Lines       27288    27764     +476     
  Branches     2785     2824      +39     
==========================================
+ Hits        23436    23897     +461     
- Misses       3167     3173       +6     
- Partials      685      694       +9
Impacted Files Coverage Δ
dipy/reconst/dti.py 96.5% <100%> (-0.01%) ⬇️
dipy/reconst/tests/test_dki.py 100% <100%> (ø) ⬆️
dipy/reconst/tests/test_dki_micro.py 100% <100%> (ø)
dipy/reconst/dki.py 96.82% <92.63%> (-0.82%) ⬇️
dipy/reconst/dki_micro.py 92.7% <92.7%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e136639...3f401d7. Read the comment docs.

@coveralls

This comment has been minimized.

coveralls commented Apr 18, 2017

Coverage Status

Coverage increased (+0.2%) to 88.563% when pulling e2d2436 on RafaelNH:DKI_microstructure_history into 568d14c on nipy:master.

@arokem

This is really close. If you have time to address the remaining comments today, I can merge this tomorrow. If anyone else wants to take a look at this, please let me know.

@@ -658,7 +780,7 @@ def _G1m(a, b, c):
"""
# Float error used to compare two floats, abs(l1 - l2) < er for l1 = l2
# Error is defined as three order of magnitude larger than system's epslon

This comment has been minimized.

@arokem

arokem Apr 18, 2017

Member

This comment is now out of date. Should be "five orders of magnitude"

@@ -720,7 +842,7 @@ def _G2m(a, b, c):
"""
# Float error used to compare two floats, abs(l1 - l2) < er for l1 = l2
# Error is defined as three order of magnitude larger than system's epslon

This comment has been minimized.

@arokem

arokem Apr 18, 2017

Member

This comment is now out of date. Should be "five orders of magnitude"

max_kurtosis : float (optional)
To keep kurtosis values within a plausible biophysical range, radial
kurtosis values that are larger than `max_kurtosis` are replaced with
`max_kurtosis`. defaut = 3
`max_kurtosis`. Default = 3

This comment has been minimized.

@arokem

arokem Apr 18, 2017

Member

Default = 10

max_kurtosis : float (optional)
To keep kurtosis values within a plausible biophysical range, mean
kurtosis values that are larger than `max_kurtosis` are replaced
with `max_kurtosis`. defaut = 3
with `max_kurtosis`. Default = 3

This comment has been minimized.

@arokem

arokem Apr 18, 2017

Member

Default = 10

Characterization with Diffusion Kurtosis Imaging. Neuroimage
58(1): 177-188. doi:10.1016/j.neuroimage.2011.06.006
"""
return trace(self.restricted_evals)

This comment has been minimized.

@arokem

arokem Apr 18, 2017

Member

Needs a check with _is_awf_only?

This comment has been minimized.

@RafaelNH

RafaelNH Apr 18, 2017

Contributor

No need! This is calling self.restricted_evals which checks _is_awf_only for it!

Before fitting this microstructural model, it is usefull to indicate the
regions in which this model provides meanfully information (i.e. voxels of
well-aligned fibers). Following Fieremans et al. [Fieremans2011]_, a simpe way
to select this region is to generate a well aligned fiber mask based on the

This comment has been minimized.

@arokem

arokem Apr 18, 2017

Member

"well aligned" => "well-aligned"

to select this region is to generate a well aligned fiber mask based on the
values of diffusion sphericity, planarity and linearity. Here we will follow
this selection criteria for a better comparision of our figures with the
orignal article publised by Fieremans et al. [Fieremans2011]_. Nevertheless, it

This comment has been minimized.

@arokem

arokem Apr 18, 2017

Member

"orignal" => "original"

values of diffusion sphericity, planarity and linearity. Here we will follow
this selection criteria for a better comparision of our figures with the
orignal article publised by Fieremans et al. [Fieremans2011]_. Nevertheless, it
is important to note that voxel well-aligned fiber can be selected based on

This comment has been minimized.

@arokem

arokem Apr 18, 2017

Member

"voxel well-aligned" => "voxels with well-aligned"

"""
The KurtosisMicrostructureFit object created by this ``fit`` function can then
be used to extract model parameters as the axonal water fraction and diffusion

This comment has been minimized.

@arokem

arokem Apr 18, 2017

Member

"... model parameters as the ..." => "...model parameters such as the..."

""" Below these parameters are ploted in top of the mean kurtosis maps: """
fig3, ax = plt.subplots(1, 2, figsize=(9, 4),

This comment has been minimized.

@arokem

arokem Apr 18, 2017

Member

Need a line of white-space between the code and the triple-quoted string

@RafaelNH

This comment has been minimized.

Contributor

RafaelNH commented Apr 18, 2017

Hi @arokem! I think I covered all your comments! Please let me know if I skipped something or if there is something else that you want me to address!

@arokem

This comment has been minimized.

Member

arokem commented Apr 18, 2017

Yep. This is ready to go from my point of view. I will merge it first thing in the morning tomorrow, unless someone else pipes up.

@coveralls

This comment has been minimized.

coveralls commented Apr 18, 2017

Coverage Status

Coverage increased (+0.2%) to 88.568% when pulling 6dfcb74 on RafaelNH:DKI_microstructure_history into 568d14c on nipy:master.

@arokem

This comment has been minimized.

Member

arokem commented Apr 19, 2017

Rebasing on master should fix the remaining failure (fixed in #1229)

@arokem arokem changed the title from (WIP) DKI Microstructural model (with correct history) to DKI Microstructural model Apr 19, 2017

RafaelNH added some commits Apr 5, 2016

TEST: Add a second test to access the performance of the function to …
…find kurtosis maxima. While the first one accessed the case of single fibers whe second test access the performance for cases of crossing fibers
Test: Add test to access the first measure already implemented in the…
… first version of the DKI microstructural model function
RF: defaults min kurtosis is always set to -3./7
According to Barmpoutis and Zhuo (2011) this is the theoretical limit for regions of water confined to spherical pores (see full ref in code)
RF, BF: Solving memory issues in diffusion_components
calculation of diffusion components are now done voxel by voxel avoiding memory related problems

arokem and others added some commits Apr 14, 2017

Split up directional kurtosis calculation into sub-functions.
This is so that we can calculate these in batch, and reuse.
RF, DOC, BF: Work on _adc and _akc. Note functions are renamed as:
_adc -> directional_diffusivity
_adv -> directional_diffusion_variance
RF: Rewrite code according to directional functions. Note ALSO
the Default of max_kurtosis in mk, ak, rk was change to 10.
NF: Raise error for cases that users calls diffusion microstructural …
…properties but only computed awf but setting awf_only to True
RF: Check number of b-values using function check_multi_b:
function can be imported from dipy.core.gradients
BF: Fix a bug in the radial_kurtosis function
When updating the gtol value of max kurtosis search procedure I changed the tolerance for detecting singularity cases of the RK by mistake. In this commit, I reset this tolerance to the right value adjusted during my GSoC project
BF: Fix bugs in decompose_tensors:
This bugs were introduced when function decompose_tensors was rewrited with directional_kurtosis, directional_diffusion and directional_diffusion_variance
TEST, BF: Fix prediction tests
before I was calling the DiffusionKurtosisModel object instead of the KurtosisMicrostructureModel
so basically dki_micro predictions functions are not working
TEST: testing the performance of kurtosis_maxima search when kurtosis…
… is spherical:

These might not be realistic however they can be problematic for kurtosis_maxima since no maxima is present
RF, BF, DOC: More tests and bugs fixed in dki_micro.py
1) min_diffusivity was added in hindered and restricted tensors decomposition
2) remove negative directional kurtosis in sqrt of diffusion_components
3) add description in doc that hindered and restricted tensors can be seen and extra and intra-cellular compartments
4) Un-needed 'if...else' structure of dki_micro.py shortened and simplified by @arokem
RF: Remove min_diffusivity in tensors decomposition and tortuosity es…
…timation:

No advantage of replacing zero values of diffusion with an minimal expected measured diffusion. Also, this was reducing the precision of the diffusion sub-components (note that this DKI microstructure model assumes that the radial diffusivity of the restricted diffusion is zero, so conserving zero diffusion improves model fit)

@RafaelNH RafaelNH force-pushed the RafaelNH:DKI_microstructure_history branch from 6dfcb74 to 3f401d7 Apr 19, 2017

@coveralls

This comment has been minimized.

coveralls commented Apr 19, 2017

Coverage Status

Coverage increased (+0.2%) to 88.562% when pulling 3f401d7 on RafaelNH:DKI_microstructure_history into e136639 on nipy:master.

@arokem

This comment has been minimized.

Member

arokem commented Apr 19, 2017

Here it goes!

@arokem arokem merged commit 53d8741 into nipy:master Apr 19, 2017

4 checks passed

codecov/patch 96.74% of diff hit (target 85.88%)
Details
codecov/project 86.07% (+0.18%) compared to e136639
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.2%) to 88.562%
Details

ShreyasFadnavis pushed a commit to ShreyasFadnavis/dipy that referenced this pull request Sep 20, 2018

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