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

Changing the default b0_threshold in gtab #1667

Merged
merged 21 commits into from Dec 6, 2018

Conversation

Projects
None yet
6 participants
@ShreyasFadnavis
Copy link
Contributor

ShreyasFadnavis commented Nov 22, 2018

More than often the b0s are not 0 and then if this is not set correctly in gradient_table that creates other problems across the pipeline for example getting response function with nans etc.

@Garyfallidis

This comment has been minimized.

Copy link
Member

Garyfallidis commented Nov 22, 2018

+1000! for this. The b0s are not necessarily exactly 0 but usually lower than 50 indeed.

@Garyfallidis

This comment has been minimized.

Copy link
Member

Garyfallidis commented Nov 22, 2018

I need a second core dev. to approve this PR. Thank you @ShreyasFadnavis and all!

@skoudoro

This comment has been minimized.

Copy link
Member

skoudoro commented Nov 22, 2018

Thank @ShreyasFadnavis for this.

Can you update b0_threshold default value on all workflows like here?

I think we should warn the user when they are doing something strange. So you should compare bval min and b0_threshold as I did on this PR and raise a warning. Can you add this behavior on this 5 functions?

@jchoude

This comment has been minimized.

Copy link
Contributor

jchoude commented Nov 22, 2018

I second the request to add a warning. Also, should there be an upper bound on b0_threshold?

@Garyfallidis

This comment has been minimized.

Copy link
Member

Garyfallidis commented Nov 22, 2018

The upper bound is acquisition dependent.

@jchoude

This comment has been minimized.

Copy link
Contributor

jchoude commented Nov 22, 2018

Yes, but I mean, would it make sense to have a B=0 threshold higher than, say, 100? It wouldn't really be a B=0 in this case, no?

@Garyfallidis

This comment has been minimized.

Copy link
Member

Garyfallidis commented Nov 22, 2018

Difficult to say. I have seen data with b0 at 100. But indeed it is rare.

@Garyfallidis

This comment has been minimized.

Copy link
Member

Garyfallidis commented Nov 22, 2018

I suppose it is safe to have an upper bound at 200 and raise a warning if it goes beyond that.

@ShreyasFadnavis

This comment has been minimized.

Copy link
Contributor

ShreyasFadnavis commented Nov 22, 2018

@skoudoro @jchoude @Garyfallidis Thank you all! So, I will add change the b0_threshold in the workflows as @skoudoro suggested. Also will add a warning and an upper bound of 200 to the b0_threshold?

@Garyfallidis

This comment has been minimized.

Copy link
Member

Garyfallidis commented Nov 22, 2018

Also a warning for a lower bound. It cannot be less than 0. 0 is the minimum. Thanks!

@skoudoro

This comment has been minimized.

Copy link
Member

skoudoro commented Nov 22, 2018

Do not forget to update the tests with this default value and add new one for the warning. Thanks!

@ShreyasFadnavis

This comment has been minimized.

Copy link
Contributor

ShreyasFadnavis commented Nov 22, 2018

Also a warning for a lower bound. It cannot be less than 0. 0 is the minimum. Thanks!

@Garyfallidis the lower bound for b0_threshold should be the lowest b0 value right? not 0 as the min, because the auto_response may give an issue in that case..

@Garyfallidis

This comment has been minimized.

Copy link
Member

Garyfallidis commented Nov 22, 2018

Nope!

@Garyfallidis

This comment has been minimized.

Copy link
Member

Garyfallidis commented Nov 22, 2018

All I am saying is check if b0_threshold is accidentally negative. Less than 0. If that is the case you should raise an Error. Sorry for not being clear multi-tasking right now with baby and laptop. But hey no excuses. Keep it up @ShreyasFadnavis !

@ShreyasFadnavis

This comment has been minimized.

Copy link
Contributor

ShreyasFadnavis commented Nov 25, 2018

I suppose it is safe to have an upper bound at 200 and raise a warning if it goes beyond that.

@Garyfallidis I am raising a warning if >= 200, but am not sure if I should bound it by replacing the actual value.

@Garyfallidis

This comment has been minimized.

Copy link
Member

Garyfallidis commented Nov 25, 2018

Nope do not replace the original value. Thank you.

ShreyasFadnavis added some commits Nov 25, 2018

@@ -65,7 +65,7 @@
500., 600., 700., 800., 900., 1000.])

bvecs_no_b0 = generate_bvecs(N)
gtab_no_b0 = gradient_table(bvals_no_b0, bvecs.T)
gtab_no_b0 = gradient_table(bvals_no_b0, bvecs.T, b0_threshold=0)

This comment has been minimized.

@arokem

arokem Nov 26, 2018

Member

The problem here is that it would break backwards compatibility for people who are already using IVIM with a gradient table initialized with the default parameters. I think that we would need to check and provide a better error for people who are using their code as it was.

This comment has been minimized.

@ShreyasFadnavis

ShreyasFadnavis Nov 26, 2018

Contributor

@arokem Thank you for this! Makes sense to me.. Should I set up a warning?

This comment has been minimized.

@skoudoro

skoudoro Nov 29, 2018

Member

I think this PR is almost ready to go. the only point is I have no clue to solve this backward compatibility problem. I do not think a warning message will solve this problem and we have already this check.

Any suggestion?

This comment has been minimized.

@arokem

arokem Nov 29, 2018

Member

I think that we need to do this within the IVIM implementation. One way to deal with this would be to check whether the gtab provided has a non-zero b0_threshold, and raise an error when that's the case, unless the user gives some other indication that that's intended. It's a bit kludgy, I'll admit it. Open to other suggestions.

This comment has been minimized.

@ShreyasFadnavis

ShreyasFadnavis Nov 30, 2018

Contributor

@arokem Will setting the default value of b0_threshold to 50 have repercussions in the functioning of IVIM for current users? I'm not sure I understand. Please could you help me with understanding this?

This comment has been minimized.

@arokem

arokem Dec 1, 2018

Member

The problem is that for IVIM, a user might very reasonably have a measurement with a b-value of 40, that should not be considered a b0 measurement. In previous versions of the software, when they ran the default code, it wasn't. But with this change introduced that measurement will now be considered to have a b-value of 0, and they wouldn't have any way of knowing that this change was introduced.

ShreyasFadnavis added some commits Nov 27, 2018

del
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Nov 27, 2018

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff            @@
##             master   #1667   +/-   ##
========================================
  Coverage          ?   84.1%           
========================================
  Files             ?     113           
  Lines             ?   13508           
  Branches          ?    2125           
========================================
  Hits              ?   11361           
  Misses            ?    1650           
  Partials          ?     497
Impacted Files Coverage Δ
dipy/reconst/ivim.py 75.29% <0%> (ø)
dipy/reconst/shm.py 86.6% <100%> (ø)
dipy/workflows/reconst.py 80.51% <100%> (ø)
dipy/core/gradients.py 89.58% <75%> (ø)

ShreyasFadnavis added some commits Dec 5, 2018

@@ -215,6 +215,9 @@ def __init__(self, gtab, split_b_D=400.0, split_b_S0=200., bounds=None,
e_s += "The IVIM model requires signal measured at 0 bvalue"
raise ValueError(e_s)

if gtab.b0_threshold > 0:
warnings.warn('The default b0_threshold > 0 and is now set to 50')

This comment has been minimized.

@arokem

arokem Dec 5, 2018

Member

I don't understand this warning.

As I user, I would infer that this means that you have changed my b0_threshold to 50.

I would propose to raise an Error (instead of the warning) with the following string:

"The IVIM model requires a measurement at b==0. As of version 0.15, the default b0_threshold for the GradientTable object is set to 50, so if you used the default settings to initialize the gtab input to the IVIM model, you may have provided a gtab with b0_threshold larger than 0. Please initialize the gtab input with b0_threshold=0"

This comment has been minimized.

@ShreyasFadnavis

ShreyasFadnavis Dec 5, 2018

Contributor

@arokem Thank you so much for this! I shall refactor accordingly!

ShreyasFadnavis added some commits Dec 5, 2018

@skoudoro

This comment has been minimized.

Copy link
Member

skoudoro commented Dec 6, 2018

Good! I think this PR is ready to be merged. I will do it this evening if there is no objection

@@ -215,6 +215,16 @@ def __init__(self, gtab, split_b_D=400.0, split_b_S0=200., bounds=None,
e_s += "The IVIM model requires signal measured at 0 bvalue"
raise ValueError(e_s)

if gtab.b0_threshold > 0:

This comment has been minimized.

@arokem

arokem Dec 6, 2018

Member

Any chance to get a test for this behavior in the IVIM tests?

This comment has been minimized.

@ShreyasFadnavis

ShreyasFadnavis Dec 6, 2018

Contributor

@arokem I will create a new issue and create a new PR with the tests for the same!

@@ -381,7 +391,7 @@ def estimate_f_D_star(self, params_f_D_star, data, S0, D):
warningMsg += " as initial guess for leastsq. Parameters are"
warningMsg += " returned only from the linear fit."
warnings.warn(warningMsg, UserWarning)
f, D_star = params_f_D
f, D_star = params_f_D_star

This comment has been minimized.

@arokem

arokem Dec 6, 2018

Member

What's this change? Was this just a bug that was hiding in there? Looks like it.

This comment has been minimized.

@ShreyasFadnavis

ShreyasFadnavis Dec 6, 2018

Contributor

@arokem yes! It was just a minor bug in the code.. I found it while reading it.

@arokem

This comment has been minimized.

Copy link
Member

arokem commented Dec 6, 2018

@arokem

This comment has been minimized.

Copy link
Member

arokem commented Dec 6, 2018

@skoudoro : +1 for the merge from me, with follow-up on separate issues/PRs.

@skoudoro

This comment has been minimized.

Copy link
Member

skoudoro commented Dec 6, 2018

ok, thank you @ShreyasFadnavis

@skoudoro skoudoro merged commit ca6e573 into nipy:master Dec 6, 2018

4 checks passed

codecov/patch No report found to compare against
Details
codecov/project No report found to compare against
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment