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: Normalization of GQI2 gqi_vector. #581

Merged
merged 1 commit into from Feb 26, 2015

Conversation

arokem
Copy link
Contributor

@arokem arokem commented Feb 26, 2015

A long overdue fix, , with thanks to @qytian for fixing this and reminding me to
put in this PR on his behalf.

Follow-up of this conversation: http://mail.scipy.org/pipermail/nipy-devel/2013-November/009574.html

A long due fix, , with thanks to @qytian for fixing this and reminding me to
put in this PR on his behalf.
@Garyfallidis
Copy link
Contributor

Hey thanks @qytian and @arokem for looking at this. Can you show as the improved ODFs in a before after figure for a 60 degrees crossing for example?

@arokem
Copy link
Contributor Author

arokem commented Feb 26, 2015

On master:

master

One this branch:
qgi-norm-fix

@arokem
Copy link
Contributor Author

arokem commented Feb 26, 2015

@Garyfallidis
Copy link
Contributor

Can you generate the same 2 figures but with sampling_length = 3?

@arokem
Copy link
Contributor Author

arokem commented Feb 26, 2015

master

qgi-norm-fix

@Garyfallidis
Copy link
Contributor

Aha!

@Garyfallidis
Copy link
Contributor

Okay thank you. I asked for this so we can have a reference on the change of sampling_length after the new pi correction.

Thank you both :)

Garyfallidis added a commit that referenced this pull request Feb 26, 2015
BF: Normalization of GQI2 `gqi_vector`.
@Garyfallidis Garyfallidis merged commit 63839f4 into dipy:master Feb 26, 2015
@arokem arokem deleted the gqi2-norm-fix branch February 26, 2015 18:22
@qytian
Copy link
Contributor

qytian commented Feb 26, 2015

The abstract "Deconvolution enhanced Generalized Q-Sampling 2 and DSI deconvolution" recommends an integration length lamda between 2 and 3. I am not sure whether here includes the "pi" constant, i.e. should be [2, 3] / pi.

@Garyfallidis
Copy link
Contributor

Yes, because it was using the old code (without your fix). @arokem and @qytian would you mind adding this information to the docstring of the function? I mean say that range of the sampling length has changed? We need also to write this in the api_changes.rst file. Thank you again.

@arokem
Copy link
Contributor Author

arokem commented Feb 26, 2015

Yeah - no problem. @qytian and I will work on another PR for that.

On Thu, Feb 26, 2015 at 10:43 AM, Eleftherios Garyfallidis <
notifications@github.com> wrote:

Yes, because it was using the old code (without your fix). @arokem
https://github.com/arokem and @qytian https://github.com/qytian would
you mind adding this information to the docstring of the function? I mean
say that range of the sampling length has changed? We need also to write
this in the api_changes.rst file. Thank you again.


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

@samuelstjean
Copy link
Contributor

Regarding the second screenshot, is that supposed to happen? You seem to have reversed the orientation of the crossings to 90 degrees on that stick figure, while it was more like 60 degrees before.

@Garyfallidis
Copy link
Contributor

Yeah, it's correct. It is the same as sampling from outside the PDF grid in DSI. Your values will not be correct or very small (in the case of GQI2) and then you may start seeing effects of the sampling sphere in the results.

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.

None yet

4 participants