[BF] IVIM fixes #1931

ShreyasFadnavis · 2019-07-29T08:57:25Z

Minor fixes, fix #1817

Added by mistake

pep8speaks · 2019-07-29T08:57:28Z

Hello @ShreyasFadnavis, Thank you for updating !

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

Comment last updated at 2019-08-01 02:01:33 UTC

codecov-io · 2019-07-29T12:23:55Z

Codecov Report

❗ No coverage uploaded for pull request base (master@f7dcb40). Click here to learn what that means.
The diff coverage is 85.71%.

@@            Coverage Diff            @@
##             master    #1931   +/-   ##
=========================================
  Coverage          ?   84.55%           
=========================================
  Files             ?      119           
  Lines             ?    14719           
  Branches          ?     2334           
=========================================
  Hits              ?    12446           
  Misses            ?     1726           
  Partials          ?      547

Impacted Files	Coverage Δ
dipy/reconst/ivim.py	`95.9% <85.71%> (ø)`

skoudoro

Overall it looks good. Just take care of you rebase because you are deleting previous work like warning management and setup_module for pytest.

Thank you @ShreyasFadnavis for this update on IVIM

skoudoro · 2019-07-29T17:13:52Z

dipy/reconst/ivim.py

@@ -10,6 +10,11 @@
 cvxpy, have_cvxpy, _ = optional_package("cvxpy")


+# global variables for bounding least_squares in both models
+BOUNDS_TRF = ([0., 0., 0., 0.], [np.inf, .2, 1., 1.])
+BOUNDS_VP = ([0.01, 0.005, 10**-4], [0.3, 0.02,  0.003])


pep8: Can you remove one space before 0.003

@skoudoro ! Thank you 👍 I have made the suggested changes...

skoudoro · 2019-07-29T17:15:13Z

dipy/reconst/tests/test_ivim.py

 needs_cvxpy = dec.skipif(not have_cvxpy)


-def setup_module():


Please, do not remove this function setup_module(). Can you update this function instead of coming back to an old version. Thank you

skoudoro · 2019-07-29T17:16:06Z

dipy/reconst/tests/test_ivim.py

@@ -337,18 +320,8 @@ def test_noisy_fit():
    around 135 and D and D_star values are equal. Hence doing a test based on
    Scipy version.
    """
-    model_one_stage = IvimModel(gtab, fit_method='LM')
-    with warnings.catch_warnings(record=True) as w:


Same here, please keep the warning management and just update the model call

skoudoro · 2019-07-29T17:16:43Z

dipy/reconst/tests/test_ivim.py

@@ -442,18 +415,7 @@ def test_leastsq_failing():
    Test for cases where leastsq fitting fails and the results from a linear
    fit is returned.
    """
-    with warnings.catch_warnings(record=True) as w:


Same comment as above, keep the warning management.

skoudoro · 2019-07-29T17:16:57Z

dipy/reconst/tests/test_ivim.py

@@ -465,17 +427,23 @@ def test_leastsq_error():
    passed. If an unfeasible x0 value is passed using which leastsq fails, the
    x0 value is returned as it is.
    """
-    with warnings.catch_warnings(record=True) as w:
-        warnings.simplefilter("always", category=UserWarning)


same comment as above

skoudoro · 2019-07-30T04:44:14Z

Thank you for the quick correction @ShreyasFadnavis!

@arokem, Can you have a quick look at this PR?

arokem

A few comments from me.

Also, why "TRF" for trust reflective region? Shouldn't that be "TRR"?

dipy/reconst/ivim.py

arokem · 2019-07-30T04:49:39Z

dipy/reconst/ivim.py


    """
+    boundsWarning = 'Bounds for this fit have been set from experiments '
+    boundsWarning += 'and literature survey. To change the bounds, please '


arokem · 2019-07-30T04:49:44Z

dipy/reconst/ivim.py


    """
+    boundsWarning = 'Bounds for this fit have been set from experiments '
+    boundsWarning += 'and literature survey. To change the bounds, please '
+    boundsWarning += 'input your bounds in model definition...'


arokem · 2019-07-30T04:50:28Z

dipy/reconst/ivim.py

-    if fit_method.lower() == 'lm':
-        return IvimModelLM(gtab, **kwargs)
+    if fit_method.lower() == 'trf':
+        warnings.warn(boundsWarning, UserWarning)


IIUC, this warning would be raised even if the user set the bounds themselves.

So this warning will always popup whatever the user does? What is the goal of this warning?

As discussed in #1817 , it is to warn the users that there is no fixed recipe for the bounds and we provide the users a way to change them.. makes sense?

ok, but when the user changes the bounds, the warning will still appear, right? Do we really want that or should we adapt? if the user changes it, no warning otherwise, a warning appears

@skoudoro You are right! I have tried to address this in my latest commit :)

arokem · 2019-07-30T04:51:06Z

dipy/reconst/ivim.py

@@ -10,6 +10,11 @@
 cvxpy, have_cvxpy, _ = optional_package("cvxpy")


+# global variables for bounding least_squares in both models
+BOUNDS_TRF = ([0., 0., 0., 0.], [np.inf, .2, 1., 1.])
+BOUNDS_VP = ([0.01, 0.005, 10**-4], [0.3, 0.02, 0.003])


I must be missing something: why are separate bounds set for each method?

arokem · 2019-07-30T04:53:09Z

dipy/reconst/ivim.py

@@ -590,7 +600,7 @@ def fit(self, data, bounds_de=None):
        x_f = self.x_and_f_to_x_f(x, f)

        # Setting up the bounds for least_squares
-        bounds = ([0.01, 0.005, 10**-4], [0.3, 0.02,  0.003])
+        bounds = BOUNDS_VP


It looks like it's overwriting the input (from the keyword argument). Shouldn't this only be set from the global variable if bounds is None ?

Co-Authored-By: Ariel Rokem <arokem@gmail.com>

ShreyasFadnavis · 2019-07-30T09:01:41Z

A few comments from me.

Also, why "TRF" for trust reflective region? Shouldn't that be "TRR"?

Fixed all that was suggested :) Let me know in case of any further changes 👍

skoudoro · 2019-07-31T02:32:03Z

Does Anyone want to add another comments ?

Can you have a last look @arokem?

I would like to merge this tomorrow at noon EST

dipy/reconst/ivim.py

arokem · 2019-08-01T02:14:06Z

OK. I believe this is ready to go!

@skoudoro : the GitHub PR says you still have requested changes, but it looks like @ShreyasFadnavis has already addressed all your comments.

If you are pleased, please go ahead and merge this.

skoudoro · 2019-08-01T02:22:32Z

will do!! as soon as Travis is happy, just waiting for that! Thank you @ShreyasFadnavis and @arokem

skoudoro · 2019-08-01T10:01:48Z

Thanks @ShreyasFadnavis and @arokem ! Merging

ShreyasFadnavis added 6 commits July 18, 2019 11:02

initial commit

d676067

changed lm to trf

9ddeded

del

085689b

Delete reconst_mcsd.py

ae472e1

Added by mistake

rem conflicts

8f41b28

bounds

169658c

ShreyasFadnavis requested a review from skoudoro July 29, 2019 09:00

fixing pep8

5e66b51

ShreyasFadnavis requested a review from arokem July 29, 2019 09:05

skoudoro added this to the 1.0 milestone Jul 29, 2019

skoudoro requested changes Jul 29, 2019

View reviewed changes

fixes

69bde0a

arokem reviewed Jul 30, 2019

View reviewed changes

ShreyasFadnavis and others added 2 commits July 30, 2019 12:50

Update dipy/reconst/ivim.py

ba978fa

Co-Authored-By: Ariel Rokem <arokem@gmail.com>

fixes suggested by Ariel

e15274c

ShreyasFadnavis force-pushed the ivim_bf branch from ba978fa to e15274c Compare July 30, 2019 08:44

fixing vol frac

58a9d73

addressing Serge's comment

7682532

arokem reviewed Jul 31, 2019

View reviewed changes

dipy/reconst/ivim.py Outdated Show resolved Hide resolved

arokem reviewed Jul 31, 2019

View reviewed changes

dipy/reconst/ivim.py Outdated Show resolved Hide resolved

arokem reviewed Jul 31, 2019

View reviewed changes

dipy/reconst/ivim.py Show resolved Hide resolved

ShreyasFadnavis added 2 commits July 31, 2019 22:24

addressing Ariel's comments

2cf1acf

adding ref comment for bounds

8442405

skoudoro merged commit 0b8708c into dipy:master Aug 1, 2019

skoudoro mentioned this pull request Aug 1, 2019

[WIP] - MTMS-CSD Tutorial #1909

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BF] IVIM fixes #1931

[BF] IVIM fixes #1931

ShreyasFadnavis commented Jul 29, 2019 •

edited by skoudoro

pep8speaks commented Jul 29, 2019 •

edited

codecov-io commented Jul 29, 2019 •

edited by codecov bot

skoudoro left a comment

skoudoro Jul 29, 2019

ShreyasFadnavis Jul 29, 2019

skoudoro Jul 29, 2019

skoudoro Jul 29, 2019

skoudoro Jul 29, 2019

skoudoro Jul 29, 2019

skoudoro commented Jul 30, 2019

arokem left a comment

arokem Jul 30, 2019

arokem Jul 30, 2019

arokem Jul 30, 2019

ShreyasFadnavis Jul 30, 2019

skoudoro Jul 30, 2019

ShreyasFadnavis Jul 30, 2019

skoudoro Jul 30, 2019

ShreyasFadnavis Jul 30, 2019

arokem Jul 30, 2019

arokem Jul 30, 2019

ShreyasFadnavis commented Jul 30, 2019

skoudoro commented Jul 31, 2019

arokem commented Aug 1, 2019

skoudoro commented Aug 1, 2019

skoudoro commented Aug 1, 2019

[BF] IVIM fixes #1931

[BF] IVIM fixes #1931

Conversation

ShreyasFadnavis commented Jul 29, 2019 • edited by skoudoro

pep8speaks commented Jul 29, 2019 • edited

Comment last updated at 2019-08-01 02:01:33 UTC

codecov-io commented Jul 29, 2019 • edited by codecov bot

Codecov Report

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

skoudoro commented Jul 30, 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

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 Jul 30, 2019

skoudoro commented Jul 31, 2019

arokem commented Aug 1, 2019

skoudoro commented Aug 1, 2019

skoudoro commented Aug 1, 2019

ShreyasFadnavis commented Jul 29, 2019 •

edited by skoudoro

pep8speaks commented Jul 29, 2019 •

edited

codecov-io commented Jul 29, 2019 •

edited by codecov bot