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

(DO NOT MERGE THIS PR) NF: DKI microstructural model #1027

Closed
wants to merge 131 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@RafaelNH
Contributor

RafaelNH commented Apr 5, 2016

Hello world! In this pull request, I am implementing the DKI based microstructural model which was suggested by (Fieremans et al., 2011).

I am started this fresh PR from the work done on the last weeks of my participation at last years Google summer of code (GSoC2015 blog). At the moment, the axonal water fraction is already implemented. The next step is to estimate the intra and extra-axonal diffusion tensors.

Hope you enjoy!

from .base import ReconstModel
from dipy.core.geometry import (sphere2cart, cart2sphere)
from .recspeed import local_maxima

This comment has been minimized.

@arokem

arokem Apr 5, 2016

Member

Use absolute imports, please.

@@ -10,8 +11,9 @@

from dipy.reconst.utils import dki_design_matrix as design_matrix
from dipy.utils.six.moves import range
from ..core.onetime import auto_attr
from .base import ReconstModel

This comment has been minimized.

@arokem

arokem Apr 5, 2016

Member

While you are at it, you can also change this one into an absolute import

@@ -359,6 +361,75 @@ def _F2m(a, b, c):
return F2


def _directional_kurtosis(dt, MD, kt, V, min_diffusivity=0, min_kurtosis=-1):

This comment has been minimized.

@arokem

arokem Apr 5, 2016

Member

Might this be useful as a public function, rather than a private helper function?

return max_value, max_direction


def single_fiber_model(dki_params, sphere, mask=None, gtol=1e-5):

This comment has been minimized.

@arokem

arokem Apr 5, 2016

Member

I think axonal_water_fraction is a more appropriate name, given the output.

RDe : ndarray (x, y, z, 27) or (n, 27)
Radial Diffusivity of extra-cellular compartment
tort : ndarray (x, y, z, 27) or (n, 27)
Tortuosity

This comment has been minimized.

@arokem

arokem Apr 5, 2016

Member

I see. You want this to eventually return all the statistics?


evals, evecs, kt = split_dki_param(dki_params)
# select non-zero voxels
# (when #677 is merged replace this code lines by function _positive_evals)

This comment has been minimized.

@arokem

arokem Apr 5, 2016

Member

Is this still relevant? It seems that #677 was merged a while back

sphere = get_sphere('symmetric724')
AWF = dki.single_fiber_model(dkiF.model_params, sphere, mask=None,
gtol=1e-5)
assert_almost_equal(AWF, fie)

This comment has been minimized.

@arokem

arokem Apr 5, 2016

Member

Does this work well with values of fie far from 0.5?

@@ -359,6 +361,75 @@ def _F2m(a, b, c):
return F2


def _directional_kurtosis(dt, MD, kt, V, min_diffusivity=0, min_kurtosis=-1):
r""" Helper function that calculate the apparent kurtosis coefficient (AKC)

This comment has been minimized.

@arokem

arokem Apr 5, 2016

Member

"calculate" => "calculates"

`min_diffusivity`. defaut = 0
min_kurtosis : float (optional)
Because high amplitude negative values of kurtosis are not physicaly
and biologicaly pluasible, and these causes huge artefacts in kurtosis

This comment has been minimized.

@arokem

arokem Apr 5, 2016

Member

Typo in "plausible".

"causes" => "cause".

"high amplitude" => "high-amplitude"

I would also drop the word "huge" (https://twitter.com/search?q=%23yuuge :-D)

min_kurtosis : float (optional)
Because high amplitude negative values of kurtosis are not physicaly
and biologicaly pluasible, and these causes huge artefacts in kurtosis
based measures, directional kurtosis values than `min_kurtosis` are

This comment has been minimized.

@arokem

arokem Apr 5, 2016

Member

"kurtosis based" => "kurtosis-based"

This comment has been minimized.

@arokem

arokem Apr 5, 2016

Member

There's a word missing here?
"...directional kurtosis values smaller than..." ?



def _kt_maxima_converge(ang, dt, MD, kt):
""" Helper function that computes the negate of the directional kurtosis

This comment has been minimized.

@arokem

arokem Apr 5, 2016

Member

With help from a English native speaking mathematician: I think the word "negate" should be "inverse". I know, that's not how it works with matrices, but apparently that's how it works with numbers.

Returns
-------
neg_kt : float
The negated values of the directional kurtosis for the given direction.

This comment has been minimized.

@arokem

arokem Apr 5, 2016

Member

Same here

This comment has been minimized.

@arokem

arokem Apr 5, 2016

Member

Apparently "additive inverse", "opposite" and "negation" will also do: https://en.wikipedia.org/wiki/Additive_inverse

dipy.reconst.dki.kurtosis_maxima
"""
n = np.array([sphere2cart(1, ang[0], ang[1])])
return - _directional_kurtosis(dt, MD, kt, n)

This comment has been minimized.

@arokem

arokem Apr 5, 2016

Member

Could you write this out as -1 * ...? That negative sign is small and easy to miss.

return - _directional_kurtosis(dt, MD, kt, n)


def kurtosis_maxima(dt, MD, kt, sphere, gtol=1e-5):

This comment has been minimized.

@arokem

arokem Apr 5, 2016

Member

This is singular, right? Maybe kurtosis_maximum?

gtol : float, optional
This input is to refine kurtosis maxima under the precision of the
directions sampled on the sphere class instance. The gradient of the
convergence procedure must be less than gtol before succesful

This comment has been minimized.

@arokem

arokem Apr 5, 2016

Member

"succesful" => "successful"

max_value : float
kurtosis tensor maxima value
max_dir : array (3,)
Cartesian coordinates of the direction of the maxima kurtosis value

This comment has been minimized.

@arokem

arokem Apr 5, 2016

Member

"maxima" => "maximal"

3) Fifteen elements of the kurtosis tensor
sphere : Sphere class instance, optional
The sphere providing sample directions for the initial search of the
maxima value of kurtosis.

This comment has been minimized.

@arokem

arokem Apr 5, 2016

Member

"maxima" => maximal

@arokem

This comment has been minimized.

Member

arokem commented Apr 5, 2016

This is great. We need to think what API we eventually want here. One idea I had is that we implement a Model and Fit object that inherit from DKI, the model fit method does this additional fitting step, and then the Fit object holds a few more parameters. What do you think about that?

@arokem

This comment has been minimized.

Member

arokem commented Apr 5, 2016

Question: I just noticed that in some places, min_kurtosis is set per-default to 0 (e.g. here: https://github.com/nipy/dipy/blob/master/dipy/reconst/dki.py#L827) and in some places it is set to -1 (e.g. here: https://github.com/nipy/dipy/blob/master/dipy/reconst/dki.py#L363). Would it make sense to have the same default everywhere? What's a more reasonable default for these? Would kurtosis ever go below that of free water (0?)?

RafaelNH added some commits May 17, 2016

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)

RafaelNH added some commits Apr 15, 2017

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
DOC: Further edits in documentation according to the comments made by @…
…arokem

Major edit, adding the definition of intra-cellular and extra-cellular in documentations

@RafaelNH RafaelNH force-pushed the RafaelNH:DKI_microstructural_model branch from 908924f to b1a78a0 Apr 16, 2017

@RafaelNH

This comment has been minimized.

Contributor

RafaelNH commented Apr 16, 2017

Hello! Here is an update of the things missing in this PR:

  • Test the errors when the hindered and restricted diffusion propreties are called but awf_only was set to True. @arokem can you help me on this? See my comments above.
  • Merge @arokem 's PR with code change suggestions (e.g. removing else if conditions in mask)
  • Add example of usage -> I will try to do this tomorrow!
@coveralls

This comment has been minimized.

coveralls commented Apr 16, 2017

Coverage Status

Coverage increased (+0.2%) to 88.582% when pulling 4f2aa5a on RafaelNH:DKI_microstructural_model into d3cb337 on nipy:master.

@coveralls

This comment has been minimized.

coveralls commented Apr 16, 2017

Coverage Status

Coverage increased (+0.2%) to 88.582% when pulling 4f2aa5a on RafaelNH:DKI_microstructural_model into d3cb337 on nipy:master.

BF, TEST: Fix tortuosity indexing error for numpy 1.7
Due to the increase complexity of tortuosity computation, its code was implemented in a separate stand alone function. Tests to further evaluate the code in different single voxel cases were added
@coveralls

This comment has been minimized.

coveralls commented Apr 16, 2017

Coverage Status

Coverage increased (+0.2%) to 88.589% when pulling fe270e1 on RafaelNH:DKI_microstructural_model into d3cb337 on nipy:master.

@arokem

This comment has been minimized.

Member

arokem commented Apr 16, 2017

This commit implements the removal of redundant if... else clauses: RafaelNH@07717e5

RafaelNH added some commits Apr 17, 2017

BF, TEST: Add test for case of isotropic kurtosis tensor:
This case can be an issue for the kurtosis_maximum finder since this does not present an maximum. Based on this test a bug was fixed
Merge pull request #3 from arokem/DKI_microstructural_model
A few suggestions on top of DKI microstructural model
Merge branch 'DKI_microstructural_model' of https://github.com/Rafael…
…NH/dipy into DKI_microstructural_model

# By RafaelNH (56) and arokem (2)
# Via GitHub (4) and Ariel Rokem (2)
* 'DKI_microstructural_model' of https://github.com/RafaelNH/dipy: (58 commits)
  Un-needed `if...else` structure shortened and simplified.
  TEST: Small adjustments in tests
  TEST, BF: Edit tests for prediction procedures and correct dki_micro_prediction errors
  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
  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
  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
  RF: Check number of b-values using function check_multi_b: function can be imported from dipy.core.gradients
  NF: Raise error for cases that users calls diffusion microstructural properties but only computed awf but setting awf_only to True
  DOC: document the changes done in the default max_kurtosis values
  RF: Rewrite code according to directional functions. Note ALSO the Default of max_kurtosis in mk, ak, rk was change to 10.
  RF, DOC, BF: Work on _adc and _akc. Note functions are renamed as: _adc -> directional_diffusivity _adv -> directional_diffusion_variance
  Split up directional kurtosis calculation into sub-functions.
  DOC: Further corrections of documentation, changing the following terms: intra-cellular diffusion - restricted diffusion extra-cellular diffusion - hindered diffusion microstructural - microstructure
  DOC: more typos corrected
  DOC: Correct typos in documentation
  RF: _directional_kurtosis to directional_kurtosis gtol=1e-5 -> gtol=1e-2 to speed performance
  TEST, BF: Add prediction funtion tests and bug fix
  TEST, BF: Add multi-voxel test for dki_micro objects fix error related to mask indexing also replace sphere default to smaller number of gradient to speed up performance
  TEST, BF: Add DKI microstructure object tests, code bug fixed
  NF: Add properties into KurtosisMicrostructuralFit object 1. axonal diffusivity 2. awf 3. restricted_evals 4. hendered_evals 5. hindered_ad 6. hindered_rd 7. tortuosity
  ...
@coveralls

This comment has been minimized.

coveralls commented Apr 17, 2017

Coverage Status

Coverage increased (+0.2%) to 88.588% when pulling a440691 on RafaelNH:DKI_microstructural_model into d3cb337 on nipy:master.

RF: Adding _min_diffusivity definition in dkimicro fit object and use…
… it for compartmental diffusion tensor estimates
@coveralls

This comment has been minimized.

coveralls commented Apr 17, 2017

Coverage Status

Coverage increased (+0.2%) to 88.595% when pulling 903c7bb on RafaelNH:DKI_microstructural_model into d3cb337 on nipy:master.

RafaelNH added some commits Apr 17, 2017

BF: Removing negative directional kurtosis in diffusion_components fu…
…nction

These negative values have to be remove because they are used inside a square root
@coveralls

This comment has been minimized.

coveralls commented Apr 18, 2017

Coverage Status

Coverage increased (+0.1%) to 88.563% when pulling 7a86bd3 on RafaelNH:DKI_microstructural_model into d3cb337 on nipy:master.

@RafaelNH RafaelNH changed the title from (WIP) NF: DKI microstructural model to (DO NOT MERGE THIS PR) NF: DKI microstructural model Apr 18, 2017

@arokem

This comment has been minimized.

Member

arokem commented Apr 19, 2017

Superseded by #1226

@arokem arokem closed this Apr 19, 2017

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