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
IVIM MIX Model (MicroLearn) #1726
Conversation
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 Report
@@ 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
|
… into ivim_workflow
@arokem @skoudoro @Garyfallidis Can you please review this PR and let me know the changes? Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
|
||
SCIPY_LESS_0_17 = (LooseVersion(scipy.version.short_version) < | ||
LooseVersion('0.17')) | ||
|
||
SCIPY_LESS_0_15 = (LooseVersion(scipy.version.short_version) < |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am ok with that
|
||
@multi_voxel_fit | ||
def fit(self, data, mask=None): | ||
""" Fit method of the Ivim model class. | ||
def fit(self, data): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happened to the mask?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done as for my last commit!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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..
dipy/reconst/ivim.py
Outdated
self.exp_phi1 = np.zeros((self.bvals.shape[0], 2)) | ||
|
||
@multi_voxel_fit | ||
def fit(self, data): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We probably want an option to pass a mask here too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done as for my last commit!
dipy/reconst/ivim.py
Outdated
""" | ||
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo: "twho" => "two"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, will fix!
dipy/reconst/ivim.py
Outdated
x_f[1:3] = x | ||
return x_f | ||
|
||
def Phi(self, x): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we use lower case here? "phi" instead of "Phi"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, will do!
def __init__(self, gtab, fit_method='VarPro', *args, **kwargs): | ||
r""" Initialize an IvimModelVP class. | ||
|
||
The IVIM model assumes that biological tissue includes a volume |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there some way to avoid the duplication of the parts of the docstring that are identical between the two models?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@skoudoro @Garyfallidis can you help me with this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally, not aware of a clean way. Future work?
dipy/reconst/ivim.py
Outdated
|
||
data_max = data.max() | ||
data = data / data_max | ||
data[data > 1] = 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this actually ever happen? I thought you just normalized by dividing by the maximum
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will take it down! I didn't realize that it was still there 👍
Really good catch!
dipy/reconst/ivim.py
Outdated
data[data > 1] = 1 | ||
b = self.bvals | ||
|
||
bounds = np.array([(0.005, 0.01), (10**-4, 0.001)]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should these be configurable? What exactly do these values bound?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just add a parameter bounds_de = None and then set the default to these values. Should be no much work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now ...
@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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @ShreyasFadnavis,
Nice Documentation on your code! Below my comments
dipy/reconst/ivim.py
Outdated
self.tol = tol | ||
self.options = options | ||
self.x_scale = x_scale | ||
self.args = args |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the goal of self.args
? It is never used
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will take it down 👍 Thank you for the catch!
dipy/reconst/ivim.py
Outdated
self.x_scale = x_scale | ||
self.args = args | ||
self.kwargs = kwargs | ||
self.fit_method = fit_method |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
self.fit_method
is never used. Do you really need this variable on this class?
dipy/reconst/ivim.py
Outdated
class IvimModelLM(ReconstModel): | ||
"""Ivim model | ||
""" | ||
def __init__(self, gtab, fit_method='LM', *args, **kwargs): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
dipy/reconst/ivim.py
Outdated
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, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
dipy/reconst/ivim.py
Outdated
@@ -509,6 +536,362 @@ def _leastsq(self, data, x0): | |||
return x0 | |||
|
|||
|
|||
class IvimModelVP(ReconstModel): | |||
|
|||
def __init__(self, gtab, fit_method='VarPro', *args, **kwargs): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as before for fit_method
and *args
else: | ||
from scipy.optimize import differential_evolution | ||
|
||
data_max = data.max() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it will be good to apply the mask here, even if 'multi_voxel_fit`will do it for you later
dipy/reconst/ivim.py
Outdated
return IvimModelLM(gtab, fit_method='LM', *args, **kwargs) | ||
|
||
elif fit_method == 'VarPro': | ||
return IvimModelVP(gtab, fit_method='LM', *args, **kwargs) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
dipy/reconst/ivim.py
Outdated
self.exp_phi1 = np.zeros((self.bvals.shape[0], 2)) | ||
|
||
@multi_voxel_fit | ||
def fit(self, data, mask=None): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mask not needed the decorator can deal with that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool!
dipy/reconst/ivim.py
Outdated
return IvimModelVP(gtab, **kwargs) | ||
|
||
else: | ||
print('The fit_method option chosen was not correct. \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you use warn
instead of print
like here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
dipy/reconst/ivim.py
Outdated
@@ -509,6 +552,346 @@ def _leastsq(self, data, x0): | |||
return x0 | |||
|
|||
|
|||
class IvimModelVP(ReconstModel): | |||
|
|||
def __init__(self, gtab): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do not forget tol
and maxiter
with their defaults values.
@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! |
Thanks @ShreyasFadnavis for addressing all the issues. merging |
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.