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 - callable response does not work #314

Merged
merged 1 commit into from
Jan 14, 2014

Conversation

MrBago
Copy link
Contributor

@MrBago MrBago commented Jan 14, 2014

This has been brought up in #313 and #218. This PR fixes the documentation (and removes a few lines of superfluous code) so we don't confuse our users.

We should also add some way of using an empirical response function with the CSD Model. I still like the API suggested in #218, but this would mean breaking backwards comparability with the last release.

for now so that it can be added back with tests and an example
@arokem
Copy link
Contributor

arokem commented Jan 14, 2014

Agreed. If someone wants this kind of functionality in the future, this will have to come back with a test!

@arokem
Copy link
Contributor

arokem commented Jan 14, 2014

Just waiting for the travis-bot to be happy with this.

arokem added a commit that referenced this pull request Jan 14, 2014
@arokem arokem merged commit c61589d into dipy:master Jan 14, 2014
@MrBago MrBago deleted the bf_remove_callable_response branch January 14, 2014 02:36
should return an ndarray with the all the signal values for the response function. The response
function will be used as deconvolution kernel ([1]_)
response : tuple
A tuple with two elements. The first is the eigen-values as an (3,)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which eigen-values are these? And where should the signal be evaluated to give the response function?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a comment about the docstring, or about the procedure used?

On Tuesday, January 14, 2014, Stefan van der Walt wrote:

In dipy/reconst/csdeconv.py:

@@ -35,12 +35,12 @@ def init(self, gtab, response, reg_sphere=None, sh_order=8, lambda_=1, tau=0
Parameters
----------
gtab : GradientTable

  •    response : tuple or callable
    
  •        If tuple, then it should have two elements. The first is the eigen-values as an (3,) ndarray
    
  •        and the second is the signal value for the response function without diffusion weighting.
    
  •        This is to be able to generate a single fiber synthetic signal. If callable then the function
    
  •        should return an ndarray with the all the signal values for the response function. The response
    
  •        function will be used as deconvolution kernel ([1]_)
    
  •    response : tuple
    
  •        A tuple with two elements. The first is the eigen-values as an (3,)
    

Which eigen-values are these? And where should the signal be evaluated to
give the response function?


Reply to this email directly or view it on GitHubhttps://github.com//pull/314/files#r8861319
.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It comes from the perspective of someone who doesn't know the functionality, but who would like to try to use this sometime (but wouldn't know how). I.e., I think the docstring can be clearer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants