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

Threshold default in QballBaseModel #1763

Closed
scott-trinkle opened this issue Mar 13, 2019 · 5 comments

Comments

@scott-trinkle
Copy link
Contributor

commented Mar 13, 2019

Is there a reason that min_signal has the default value of 1. in the QballBaseModel class (and possibly elsewhere)?

The data I am working are scaled between [0, 1], so using the default parameters when reconstructing with CsaOdfModel results in identical ODFs at every voxel, with the first SH coefficient = .5 / sqrt(pi) and all others being around 0. Setting min_signal = 1e-3 or so fixes the problem.

I had to dig around a lot to find the source of the problem, I think setting a new default could save others some time, unless there is a specific reason it is set so high.

@jchoude

This comment has been minimized.

Copy link
Contributor

commented Mar 13, 2019

Out of curiosity, is the normalization to the [0, 1] range done at data conversion time, or later on in your processing?

@scott-trinkle

This comment has been minimized.

Copy link
Contributor Author

commented Mar 13, 2019

the "rawest" version of the specific dataset I'm using came off the scanner as floats in [-1.12, 1.12] (not actually [0, 1]) to my knowledge, but I didn't acquire the data myself. At this point I'm not doing any normalization myself.

@scott-trinkle

This comment has been minimized.

Copy link
Contributor Author

commented Mar 13, 2019

actually, I misspoke, the raw data is 16-bit uints. The normalization to [-1.12,1.12] happened sometime during denoising, registration, etc.

Either way: I still think setting the threshold default lower might be a good idea, I can imagine others might have the same problem in the future.

@scott-trinkle

This comment has been minimized.

Copy link
Contributor Author

commented Mar 13, 2019

Looking at the code a bit more, I see that min_signal also defaults to 1 in ResidualBootstrapWrapper (also in dipy/reconst/shm.py). This class says in the documentation:

...We assume that the signals are
normalized so we clip the bootsrap samples to be between min_signal
and 1...

so the default is to clip to [1,1], which should probably also change.

@skoudoro

This comment has been minimized.

Copy link
Member

commented May 3, 2019

fixed by #1772, closing

@skoudoro skoudoro closed this May 3, 2019

@skoudoro skoudoro added this to the 1.0 milestone May 3, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.