Bago dti min #12
Bago dti min #12
Conversation
ENH: Backport importlib
And replaced with `move_streamlines`, but I am defering that until I get a sense of what is happening here.
Maybe use dipy.sims.voxel directly?
…on of streamlines.
BF: Relative import
Use importlib instead of __import__
|
I don't think this works. You cannot pass a test simply by changing the test :). The whole issue here is that we could, and sometimes do get voxels with zeros. The case with all zeros should be considered masked, there is nothing else we can really do with that, but I'd prefer not to have to deal with it explicitly inside of the loop, that's a pretty big performance penalty. The whole point of the other test case was to show the shortcomings of the current code. Yes a voxel with very low signals will have high diffusivity. A dwi signal of 0 should be "very low signal" and give a similar result. By changing the test, you've stopped testing the very case I'm concerned about. My proposal is to revert the code, and handle this in the TensorModel instead of the fit method. This has the added benefit of reducing code duplication because we do not need handle this repeatedly for each fit_method. |
|
OK - I am just trying to understand the concern. Will keep hacking on On Wed, Dec 10, 2014 at 2:10 PM, MrBago notifications@github.com wrote:
|
|
Oh - one more detail: what should the eigenvalue be for those voxels (all I guess it should be isotropic, so the eigen-vectors is np.eye(3), but what On Wed, Dec 10, 2014 at 2:14 PM, Ariel Rokem arokem@gmail.com wrote:
|
|
Is this too much? It deals with the whole issue up-front, regardless of the fitting Not all tests are passing, but I think it might be doing the right thing On Wed, Dec 10, 2014 at 2:18 PM, Ariel Rokem arokem@gmail.com wrote:
|
|
OK - I believe that the tests should be passing now (they're passing on my laptop). We still need to refactor out all the references to min_signal, outside of the |
|
Not the approach that I would have taken :), but it does solve the problem. Bago |
…ly very low, evals are very high.
These two cases should be separated: if s0 is non-zero, and all of the d_sig is zero, the eigenvalues should be high (3.0 per default, the diffusion of water in water at 37 deg C). If all of the signals are 0, we set the eigenvalues and eigenvectors to 0.
Also, deal with setting of min_signal. This still needs a little bit more cleanup.
|
How about I rebase this stuff on master and make a new PR? |
|
Sure, I believe with this approach we should be able to remove all Bago On Thu, Dec 11, 2014 at 11:18 AM, Ariel Rokem notifications@github.com
|
|
Yes - we can keep it around at the init level, so people can use it if On Thu, Dec 11, 2014 at 11:21 AM, MrBago notifications@github.com wrote:
|
|
I decided to go with the more extreme variant of this. What do you think? On Thu, Dec 11, 2014 at 11:25 AM, Ariel Rokem arokem@gmail.com wrote:
|
DTI requires a b0 measurement. This did not exist in the data that was previously used for testing. For that reason, the eigenvalues in these tests were always exceedingly low (set by min_diffusivity, I believe). I believe that it makes more sense to test the model fitting with DTI data. Single b-value and including a b0 measurement.
|
@arokem I'd like to talk about this, can you contact me offline? |
Almost there - still need to deal with that hard-coded minimum for cases where all the signal is 0. Need to think about that.
And change back the script. Whew!
There are too many commits here, but some of these should go away when you rebase your branch on master. It's the recent merge of the LiFE branch.
The last commit is what I am proposing. How about that? I have set it so that a voxel with 0 signal is considered to be masked (there's something wrong with that one anyway, no?) and a signal with a very low signal has high eigen-values. What do you think? Does this solve your issue? Any other ideas?