IVIM MIX Model (MicroLearn) #1726

ShreyasFadnavis · 2019-01-24T01:39:46Z

First Model from the MicroLearn Framework in DIPY -- IVIM Fitting using Variable Projections

Fadnavis S, M. Reisert, H. Farooq, M. Afzali, C. Hu, B. Amirbekian, E. Garyfallidis, MicroLearn: Framework for machine learning, reconstruction, optimization and microstructure modeling, Proceedings of: International Society of Magnetic Resonance in Medicine (ISMRM), Montreal, Canada, 2019.

Improve the documentation for VarPro Framework

pep8speaks · 2019-01-24T22:58:28Z

Hello @ShreyasFadnavis, Thank you for updating !

Cheers ! There are no PEP8 issues in this Pull Request. 🍻

Comment last updated at 2019-03-07 19:56:36 UTC

codecov-io · 2019-01-24T22:58:29Z

Codecov Report

Merging #1726 into master will decrease coverage by 0.43%.
The diff coverage is 91.76%.

@@            Coverage Diff             @@
##           master    #1726      +/-   ##
==========================================
- Coverage   84.26%   83.82%   -0.44%     
==========================================
  Files         115      118       +3     
  Lines       13606    14227     +621     
  Branches     2144     2252     +108     
==========================================
+ Hits        11465    11926     +461     
- Misses       1643     1781     +138     
- Partials      498      520      +22

Impacted Files	Coverage Δ
dipy/sims/voxel.py	`90.58% <ø> (ø)`	⬆️
dipy/reconst/dti.py	`96.5% <ø> (ø)`	⬆️
dipy/reconst/ivim.py	`84% <91.76%> (+3.41%)`	⬆️
dipy/workflows/align.py	`91.59% <0%> (-3.86%)`	⬇️
dipy/reconst/forecast.py	`90.15% <0%> (-2.08%)`	⬇️
dipy/reconst/mapmri.py	`90.33% <0%> (-0.63%)`	⬇️
dipy/workflows/reconst.py	`76.89% <0%> (-0.6%)`	⬇️
dipy/align/streamlinear.py	`87.17% <0%> (-0.05%)`	⬇️
dipy/io/image.py	`100% <0%> (ø)`	⬆️
dipy/data/__init__.py	`82.05% <0%> (ø)`	⬆️
... and 10 more

…master

… into ivim_workflow

…to ivim_mix

ShreyasFadnavis · 2019-03-07T01:17:57Z

@arokem @skoudoro @Garyfallidis Can you please review this PR and let me know the changes?

Thanks!

arokem

Very well done! I had a few small comments and questions here and there. My main comment is that I would like to see this incorporated into the example. I think that a comparison between the two models (e.g., in terms of model fit accuracy) would be really interesting and should be easy to incorporate into the existing example.

arokem · 2019-03-06T00:06:03Z

dipy/reconst/ivim.py


 SCIPY_LESS_0_17 = (LooseVersion(scipy.version.short_version) <
                   LooseVersion('0.17'))

+SCIPY_LESS_0_15 = (LooseVersion(scipy.version.short_version) <


Should we increase the minimal scipy required? 0.15 was released in 2014, so maybe it's fine to go with that as a minimal supported version? @skoudoro, @Garyfallidis: what do you think?

I am ok with that

arokem · 2019-03-07T06:03:15Z

dipy/reconst/ivim.py


    @multi_voxel_fit
-    def fit(self, data, mask=None):
-        """ Fit method of the Ivim model class.
+    def fit(self, data):


What happened to the mask?

Done as for my last commit!

The @multi_voxel decorator I believe adds the mask and does not need to be included in the fit I guess.. Does that make sense?

-- This is the reason I had taken it down..

arokem · 2019-03-07T06:04:15Z

dipy/reconst/ivim.py

+        self.exp_phi1 = np.zeros((self.bvals.shape[0], 2))
+
+    @multi_voxel_fit
+    def fit(self, data):


We probably want an option to pass a mask here too.

Done as for my last commit!

arokem · 2019-03-07T06:05:46Z

dipy/reconst/ivim.py

+        """
+        Creates a structure for the combining the diffusion and pseudo-
+        diffusion by multipling with the bvals and then exponentiating each of
+        the twho components for fitting as per the IVIM- two compartment model.


Typo: "twho" => "two"

Thanks, will fix!

arokem · 2019-03-07T06:06:08Z

dipy/reconst/ivim.py

+        x_f[1:3] = x
+        return x_f
+
+    def Phi(self, x):


Could we use lower case here? "phi" instead of "Phi"?

Yes, will do!

doc/examples/reconst_ivim.py

arokem · 2019-03-07T06:12:15Z

dipy/reconst/ivim.py

+    def __init__(self, gtab, fit_method='VarPro', *args, **kwargs):
+        r""" Initialize an IvimModelVP class.
+
+        The IVIM model assumes that biological tissue includes a volume


Is there some way to avoid the duplication of the parts of the docstring that are identical between the two models?

@skoudoro @Garyfallidis can you help me with this?

Personally, not aware of a clean way. Future work?

arokem · 2019-03-07T06:13:23Z

dipy/reconst/ivim.py

+
+        data_max = data.max()
+        data = data / data_max
+        data[data > 1] = 1


Does this actually ever happen? I thought you just normalized by dividing by the maximum

Will take it down! I didn't realize that it was still there 👍
Really good catch!

arokem · 2019-03-07T06:14:26Z

dipy/reconst/ivim.py

+        data[data > 1] = 1
+        b = self.bvals
+
+        bounds = np.array([(0.005, 0.01), (10**-4, 0.001)])


Should these be configurable? What exactly do these values bound?

Sorry, I forgot to add documentation for this!

I will do so immediately. And, I do not want them to be configurable because the behavior of differential evolution would become intractable! I have set the parameters after a lot of trial and error!

In the coming days, I plan to Cythonize some parts of the code and then will play around with the parameters again, but for now, due to speed issues and stochastic nature of the optimization method, I want to keep them fixed!

Just add a parameter bounds_de = None and then set the default to these values. Should be no much work.

For now ...

ShreyasFadnavis · 2019-03-07T12:49:46Z

Very well done! I had a few small comments and questions here and there. My main comment is that I would like to see this incorporated into the example. I think that a comparison between the two models (e.g., in terms of model fit accuracy) would be really interesting and should be easy to incorporate into the existing example.

@arokem Thank you so much for reviewing my PR!

Regarding the example, I have already created one, but need more time to write the documentation for it since the method is a little different. Here is the link to the example notebook for the MIX method of IVIM that I have created: Comparison Example for IVIM DIPY Workshop

It exactly replicates your example in http://nipy.org/dipy/examples_built/reconst_ivim.html#example-reconst-ivim for ready comparison.. I am working on creating a notebook where your and my example results will be placed side by side.

I want this PR to go into the release branch 0.16 and felt that merging the example would get complicated. I will open up a PR for it shortly!

skoudoro

Hi @ShreyasFadnavis,

Nice Documentation on your code! Below my comments

skoudoro · 2019-03-07T14:12:19Z

dipy/reconst/ivim.py

-        self.tol = tol
-        self.options = options
-        self.x_scale = x_scale
+        self.args = args


What is the goal of self.args ? It is never used

Will take it down 👍 Thank you for the catch!

skoudoro · 2019-03-07T14:13:59Z

dipy/reconst/ivim.py

-        self.x_scale = x_scale
+        self.args = args
+        self.kwargs = kwargs
+        self.fit_method = fit_method


self.fit_methodis never used. Do you really need this variable on this class?

skoudoro · 2019-03-07T14:14:34Z

dipy/reconst/ivim.py

+class IvimModelLM(ReconstModel):
+    """Ivim model
+    """
+    def __init__(self, gtab, fit_method='LM', *args, **kwargs):


What is the goal of fit_method in this class? What is the goal of *args in this class?

Furthermore, I think it will be better to be explicit and put all keywords arguments instead of **kwargs.

skoudoro · 2019-03-07T14:24:33Z

dipy/reconst/ivim.py

+        self.bounds = kwargs.get('bounds', None)
+        self.two_stage = kwargs.get('two_stage', True)
+        self.tol = kwargs.get('tol', 1e-15)
+        self.options = kwargs.get('options', {'gtol': 1e-15, 'ftol': 1e-15,


What happens if I give this parameter: options={'eps': 1e15} ?

The user does not know what keyword should be in options. This is why I think it will be better to be explicit on the __init__and then, build your options variable.

skoudoro · 2019-03-07T14:26:06Z

dipy/reconst/ivim.py

@@ -509,6 +536,362 @@ def _leastsq(self, data, x0):
                return x0


+class IvimModelVP(ReconstModel):
+
+    def __init__(self, gtab, fit_method='VarPro', *args, **kwargs):


same as before for fit_method and *args

skoudoro · 2019-03-07T14:28:11Z

dipy/reconst/ivim.py

+        else:
+            from scipy.optimize import differential_evolution
+
+        data_max = data.max()


I think it will be good to apply the mask here, even if 'multi_voxel_fit`will do it for you later

skoudoro · 2019-03-07T14:34:28Z

dipy/reconst/ivim.py

+        return IvimModelLM(gtab, fit_method='LM', *args, **kwargs)
+
+    elif fit_method == 'VarPro':
+        return IvimModelVP(gtab, fit_method='LM', *args, **kwargs)


it would be good to have an else in case there is an error on the fit_method.

You can call again IvimModelLM(gtab, fit_method='LM', *args, **kwargs)and add a warning for the wrong choice, selecting the default method

Garyfallidis · 2019-03-07T16:17:27Z

dipy/reconst/ivim.py

+        self.exp_phi1 = np.zeros((self.bvals.shape[0], 2))
+
+    @multi_voxel_fit
+    def fit(self, data, mask=None):


mask not needed the decorator can deal with that.

skoudoro · 2019-03-07T19:20:00Z

dipy/reconst/ivim.py

+        return IvimModelVP(gtab, **kwargs)
+
+    else:
+        print('The fit_method option chosen was not correct. \


Can you use warn instead of print like here

skoudoro · 2019-03-07T19:21:57Z

dipy/reconst/ivim.py

@@ -509,6 +552,346 @@ def _leastsq(self, data, x0):
                return x0


+class IvimModelVP(ReconstModel):
+
+    def __init__(self, gtab):


Do not forget tol and maxiter with their defaults values.

ShreyasFadnavis · 2019-03-08T02:41:18Z

@arokem I have incorporated all the changes suggested by you, @skoudoro and @Garyfallidis .. Please can you take a look at it and merge if possible?

Let me know if you need any further information on my end!

skoudoro · 2019-03-08T18:50:59Z

Thanks @ShreyasFadnavis for addressing all the issues. merging

ShreyasFadnavis added 3 commits January 23, 2019 20:18

initial commit IVIM_MIX

4afa78a

cleaned code

30666f3

added example IVIM_mix WIP

9f864ef

ShreyasFadnavis mentioned this pull request Jan 25, 2019

Upgrade Scipy for MIX framework? #1727

Closed

ShreyasFadnavis added 24 commits January 28, 2019 18:05

added data components

35ae8bc

added data files

8f84c70

backup

df28164

wf_minor fix

0b41c4b

updated IVIM mix example

536e620

IVIM Workflow initial commit

27258e8

fixed pep8

f552ee8

added bin

7ca26bc

minor fixes

9892b02

IVIM WF working full brain

3f7fc1c

Merge branch 'master' of https://github.com/nipy/dipy into nipy-dipy-…

b31793a

…master

Merge branch 'ivim_workflow' of https://github.com/ShreyasFadnavis/dipy…

a93d2a0

… into ivim_workflow

sims, data and test

2df92f1

data for test

75da592

unchanged

77fb8db

working example

6aaeaf3

Merge branch 'ivim_mix' of https://github.com/ShreyasFadnavis/dipy in…

94cdf4b

…to ivim_mix

mix added to main IVIM

77b380a

remodel

d00c5b6

rem example and combine model

6c498e3

cleanup

6ea7cea

ivim_mix

30b6924

reverse some changes

13603af

fit and args

4e6d0df

ShreyasFadnavis added 4 commits March 4, 2019 18:07

refactor

f953f3b

fixed CVXPY

3648f08

fixed SciPy Versioning

32ba61c

added tests

b5b2053

ShreyasFadnavis changed the title ~~[WIP] IVIM MIX Model~~ IVIM MIX Model (MicroLearn) Mar 5, 2019

ShreyasFadnavis added 6 commits March 5, 2019 17:40

fixed test errors

fce6b23

remodeling

7367f97

remodeled ivim interface

2d87000

fixing tests

1ed725d

added documentation

f538284

added documentation

dde3cc2

arokem reviewed Mar 7, 2019

View reviewed changes

fixes suggested by Ariel

50656f1

skoudoro reviewed Mar 7, 2019

View reviewed changes

Garyfallidis reviewed Mar 7, 2019

View reviewed changes

ShreyasFadnavis added 2 commits March 7, 2019 12:01

fixes suggested by Elef and Serge

dafcad0

minor fix in selector

0b017e7

skoudoro reviewed Mar 7, 2019

View reviewed changes

ShreyasFadnavis added 3 commits March 7, 2019 14:27

warning in selector

438445b

explicit decl in VP model

aa4f43e

minor fixes

1420d12

skoudoro mentioned this pull request Mar 8, 2019

REL: 0.16.0 #1750

Merged

15 tasks

skoudoro merged commit 7977f2b into dipy:master Mar 8, 2019

arokem mentioned this pull request May 2, 2019

Unusual behavior in Dipy IVIM implementation/example #1817

Closed

5 tasks

IVIM MIX Model (MicroLearn) #1726

IVIM MIX Model (MicroLearn) #1726

Conversation

ShreyasFadnavis commented Jan 24, 2019 • edited

pep8speaks commented Jan 24, 2019 • edited

Comment last updated at 2019-03-07 19:56:36 UTC

codecov-io commented Jan 24, 2019 • edited

Codecov Report

ShreyasFadnavis commented Mar 7, 2019

arokem left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShreyasFadnavis Mar 7, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShreyasFadnavis Mar 7, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShreyasFadnavis Mar 7, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShreyasFadnavis commented Mar 7, 2019 • edited

skoudoro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShreyasFadnavis commented Mar 8, 2019

skoudoro commented Mar 8, 2019

ShreyasFadnavis commented Jan 24, 2019 •

edited

pep8speaks commented Jan 24, 2019 •

edited

codecov-io commented Jan 24, 2019 •

edited

ShreyasFadnavis Mar 7, 2019 •

edited

ShreyasFadnavis Mar 7, 2019 •

edited

ShreyasFadnavis Mar 7, 2019 •

edited

ShreyasFadnavis commented Mar 7, 2019 •

edited