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

RF - move around some of the predict stuff #415

Merged
merged 5 commits into from Sep 23, 2014

Conversation

Projects
None yet
2 participants
@MrBago
Contributor

MrBago commented Sep 11, 2014

Just a little bit of cleanup of the predict code. @arokem can you look over this when you have time.

@MrBago MrBago force-pushed the MrBago:fixup_predict branch from 03fb322 to 1616f8f Sep 11, 2014

"""
if R is not None:
raise ValueError("using R in this function is no longer allowed")

This comment has been minimized.

@arokem

arokem Sep 11, 2014

Member

This is playing it particularly safe. I don't think that this code has even been part of any release of dipy yet.

return pred_sig
model = ConstrainedSphericalDeconvModel(gtab, response, sh_order=sh_order)
return model.predict(sh_coeff)

This comment has been minimized.

@arokem

arokem Sep 11, 2014

Member

OK - yeah - this looks pretty good. The idea here is that any model that inherits from ConstrainedSphericalDeconvModel will then be able to do this prediction stuff?

This comment has been minimized.

@MrBago

MrBago Sep 19, 2014

Contributor

I don't quite get the question, but I think yes. Subclasses will get the predict method of the CSDmodel and can override it if it's appropriate. Calling fit.predict on any fit defers to the model which makes initiative sense, you apply a diffusion "model" to the fit to predict the signal. It also makes coding sense, many models can return the same type of fit object (ie CSDmodel and QballModel both return an SphHarmFit) and predict will work in both cases because the fit just defers to the model that created it.

I think this module level function is superfluous, I'm happy to remove it altogether if you do not need it.

This comment has been minimized.

@arokem

arokem Sep 19, 2014

Member

Yes - agreed. In that case, should also eliminate the tests of that
function around:
https://github.com/nipy/dipy/blob/master/dipy/reconst/tests/test_csdeconv.py#L283

On Fri, Sep 19, 2014 at 10:31 AM, MrBago notifications@github.com wrote:

In dipy/reconst/csdeconv.py:

  • if np.iterable(S0):
  •    # If it's an array, we need to give it one more dimension:
    

- S0 = S0[..., None]

  • This is the key operation: convolve and multiply by S0:

- pre_pred_sig = S0 * np.dot(predict_matrix, sh_coeff)

  • Now put everything in its right place:

  • pred_sig = np.zeros(pre_pred_sig.shape[:-1] + (gtab.bvals.shape[0],))
  • pred_sig[..., ~gtab.b0s_mask] = pre_pred_sig

- pred_sig[..., gtab.b0s_mask] = S0

- return pred_sig

  • model = ConstrainedSphericalDeconvModel(gtab, response, sh_order=sh_order)
  • return model.predict(sh_coeff)

I don't quite get the question, but I think yes. Subclasses will get the
predict method of the CSDmodel and can override it if it's appropriate.
Calling fit.predict on any fit defers to the model which makes initiative
sense, you apply a diffusion "model" to the fit to get the signal. It also
makes coding sense, many models can return the same type of fit object (ie
CSDmodel and QballModel both return an ShHarmFit) and predict will work in
both cases because the fit just defers to the model that created it.

I think this module level function is superfluous, I'm happy to remove it
altogether if you do not need it.


Reply to this email directly or view it on GitHub
https://github.com/nipy/dipy/pull/415/files#r17797444.

@arokem

This comment has been minimized.

Member

arokem commented Sep 19, 2014

Just checking back in here. In general this looks good, but I was just checking back to see that I understand the logic of that last move, before merging this.

@MrBago

This comment has been minimized.

Contributor

MrBago commented Sep 19, 2014

@arokem took out the function and the test. Let me know if there is anything else.

@arokem

This comment has been minimized.

Member

arokem commented Sep 20, 2014

Sorry - that was overkill! I meant for you to just take out the two lines
in the test that were looking specifically at the outcome of this
particular function. We still want to test csd.predict and
csd_fit.predict!

On Fri, Sep 19, 2014 at 3:47 PM, MrBago notifications@github.com wrote:

@arokem https://github.com/arokem took out the function and the test.
Let me know if there is anything else.


Reply to this email directly or view it on GitHub
#415 (comment).

@MrBago MrBago force-pushed the MrBago:fixup_predict branch from 33eba5f to eca598f Sep 23, 2014

RF - get rid of csd_predict function
TST - improve tests for CSDModel.predict

@MrBago MrBago force-pushed the MrBago:fixup_predict branch from eca598f to 667cfda Sep 23, 2014

@MrBago

This comment has been minimized.

Contributor

MrBago commented Sep 23, 2014

@arokem this should be better. I've added a little bit to the predict tests.

arokem added a commit that referenced this pull request Sep 23, 2014

Merge pull request #415 from MrBago/fixup_predict
RF - move around some of the predict stuff

@arokem arokem merged commit 9881004 into nipy:master Sep 23, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment