Skip to content
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

[BF] IVIM fixes #1931

Merged
merged 14 commits into from Aug 1, 2019

Conversation

@ShreyasFadnavis
Copy link
Member

commented Jul 29, 2019

Minor fixes, fix #1817

del
Delete reconst_mcsd.py
Added by mistake
@pep8speaks

This comment has been minimized.

Copy link

commented Jul 29, 2019

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

@ShreyasFadnavis ShreyasFadnavis requested a review from skoudoro Jul 29, 2019

@ShreyasFadnavis ShreyasFadnavis requested a review from arokem Jul 29, 2019

@codecov-io

This comment has been minimized.

Copy link

commented Jul 29, 2019

Codecov Report

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

Impacted file tree graph

@@            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 skoudoro added this to the 1.0 milestone Jul 29, 2019

@skoudoro
Copy link
Member

left a comment

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

@@ -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])

This comment has been minimized.

Copy link
@skoudoro

skoudoro Jul 29, 2019

Member

pep8: Can you remove one space before 0.003

This comment has been minimized.

Copy link
@ShreyasFadnavis

ShreyasFadnavis Jul 29, 2019

Author Member

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

needs_cvxpy = dec.skipif(not have_cvxpy)


def setup_module():

This comment has been minimized.

Copy link
@skoudoro

skoudoro Jul 29, 2019

Member

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

@@ -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:

This comment has been minimized.

Copy link
@skoudoro

skoudoro Jul 29, 2019

Member

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

@@ -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:

This comment has been minimized.

Copy link
@skoudoro

skoudoro Jul 29, 2019

Member

Same comment as above, keep the warning management.

@@ -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)

This comment has been minimized.

Copy link
@skoudoro

skoudoro Jul 29, 2019

Member

same comment as above

@skoudoro

This comment has been minimized.

Copy link
Member

commented Jul 30, 2019

Thank you for the quick correction @ShreyasFadnavis!

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

@arokem
Copy link
Member

left a comment

A few comments from me.

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

dipy/reconst/ivim.py Outdated Show resolved Hide resolved
"""
boundsWarning = 'Bounds for this fit have been set from experiments '
boundsWarning += 'and literature survey. To change the bounds, please '

This comment has been minimized.

Copy link
@arokem

arokem Jul 30, 2019

Member

Same here

"""
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...'

This comment has been minimized.

Copy link
@arokem

arokem Jul 30, 2019

Member

same here

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

This comment has been minimized.

Copy link
@arokem

arokem Jul 30, 2019

Member

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

This comment has been minimized.

Copy link
@ShreyasFadnavis

ShreyasFadnavis Jul 30, 2019

Author Member

Yes!

This comment has been minimized.

Copy link
@skoudoro

skoudoro Jul 30, 2019

Member

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

This comment has been minimized.

Copy link
@ShreyasFadnavis

ShreyasFadnavis Jul 30, 2019

Author Member

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?

This comment has been minimized.

Copy link
@skoudoro

skoudoro Jul 30, 2019

Member

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

This comment has been minimized.

Copy link
@ShreyasFadnavis

ShreyasFadnavis Jul 30, 2019

Author Member

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

@@ -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])

This comment has been minimized.

Copy link
@arokem

arokem Jul 30, 2019

Member

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

@@ -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

This comment has been minimized.

Copy link
@arokem

arokem Jul 30, 2019

Member

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 ?

ShreyasFadnavis and others added 2 commits Jul 30, 2019
Update dipy/reconst/ivim.py
Co-Authored-By: Ariel Rokem <arokem@gmail.com>

@ShreyasFadnavis ShreyasFadnavis force-pushed the ShreyasFadnavis:ivim_bf branch from ba978fa to e15274c Jul 30, 2019

@ShreyasFadnavis

This comment has been minimized.

Copy link
Member Author

commented Jul 30, 2019

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

This comment has been minimized.

Copy link
Member

commented Jul 31, 2019

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 Outdated Show resolved Hide resolved
dipy/reconst/ivim.py Outdated Show resolved Hide resolved
@arokem

This comment has been minimized.

Copy link
Member

commented Aug 1, 2019

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

This comment has been minimized.

Copy link
Member

commented Aug 1, 2019

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

@skoudoro

This comment has been minimized.

Copy link
Member

commented Aug 1, 2019

Thanks @ShreyasFadnavis and @arokem ! Merging

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

2 of 3 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
Codacy/PR Quality Review Up to standards. A positive pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@skoudoro skoudoro referenced this pull request Aug 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.