NF: Free water tensor model#835
Conversation
|
Looks like you're following the structure from DKI, right? That seems like a good way to start to me. I'll keep checking back to see if I can help out. |
|
Fortunately or unfortunately I'm getting some pressure to get working on this. @RafaelNH if you're otherwise occupied I'll forge on ahead and try to create my own fitting routine to merge into here. |
|
Hi @etpeterson! I have to confess that currently I cannot work on this full time, so feel free to carry with the work done here. Actually, github is a perfect platform for developers to work simultaneously in the same scripts. Just keep me updated with your progress to avoid duplicated work. We also have the advantage of being in different time zones, thus if the end of the day you upload your work, I will start my day reviewing your changes and carrying on from what you did. |
|
PS: I will be working on this in the following 2 hours. |
|
I'll just add for the benefit of both of you that you can also use our I've found it useful for coordinating work with others when working On Wed, Jan 20, 2016 at 2:34 AM, Rafael Neto Henriques <
|
|
I am getting close to a final version of the WLLS solution of the water elimination model :). It runs without problem, however its outputs are not what I would expect (the new added test is causing Travis to fail). Anyway, please let me know if you have suggestions on what was done so far or if you detect the problem that is making the test fail. Also coding contributions are welcome. Above, are the parts of the module that still have to be done:
|
|
Nice work. I copied yours and marked it up with changes and added the prediction function. Until I figure out how to contribute like a normal person I put it in my fork (https://github.com/etpeterson/dipy/blob/fwdti/dipy/reconst/fwdti.py). I'm not sure I totally followed the steps so some comments may be off base, but I do think the piterations loop is zooming in to the wrong range of f-values. |
|
Hi @etpeterson, thanks for your input :). Give a look to Dipy's documentation on how you can contribute with code http://nipy.org/dipy/devel/gitwash/index.html (@arokem, @Garyfallidis - do you have some recommendations of further reading for Eric?). You even can create a pull request of my branch. In this way the revision of your changes will be much more easy. Also it gives me the possibility of commenting code lines using tools of the github website (the comments directly to python should be only the info to stay with the final version of the files for guiding future users). Learning the basics of github is also important because is the only way for formally adding your contributions to Dipy's developing history. Anyway, I give a look on that you did. I didn't include the function apparent_diffusion_coef - it is a duplicate of function in dti (users can call it from there). The prediction function seems to be correct, however we save to test in using nosetest (see test_fwdti,py for what I tested so far). Regarding to your in script comments, I will answer them here using Github website tools. |
dipy/reconst/fwdti.py
Outdated
There was a problem hiding this comment.
@etpeterson - this is already correct. First right side equation term already is multiplying by S0, because last element of all_new_params is log(S0), i.e:
np.exp(np.dot(W, all_new_params)) = S0*np.exp(np.dot(W[:, :6], all_new_params[:6]))
Second term FS_S0r_SFW.T needs S0 since the free water contribution computed in line 363 do not take into account S0.
|
Looks great @RafaelNH! I just got it running on some data I have and at first glance the results look reasonable and it runs just fine. I'll test it some more later and let you know. |
dipy/reconst/fwdti.py
Outdated
There was a problem hiding this comment.
elinimation -> elimination
|
Hi @RafaelNH, this looks like going towards the correct direction. For merging this soon you need to increase your coverage and report some timings and images with full datasets. After this is done you will need to go ahead with a tutorial. This can be in a second PR. |
|
Also, you may want to ask from @etpeterson to write or extend the tutorial about this and you can mentor him a bit. In that way he can learn how to contribute to Dipy. Sounds like a good plan @etpeterson ? |
|
That works for me @Garyfallidis |
|
@Garyfallidis, @etpeterson - Ok! Added some more tests! The only lines that I am not covering is when lsq non-linear procedure does not converge. Anyone have any idea how to test this? Anyway, even if this lines are not working property, it should not be problematic. Since fw_params is already set to the initial guess parameters at the beginning of the function, these lines of code are redundant (perhaps we can even remove the non-covered lines, I was keeping them only to be more consistent to the restore function in dti.py). I think the next step for me will be running this function in some real data and time its speed. After that I will be more than happy to help @etpeterson with a new PR for the tutorial =). How that sounds? |
|
Sounds awesome! Fly high! :) |
|
I used the fwdti model to process a slice of the CENIR multi b-values dataset. It took around 90 secs to be processed. Thus, to process the full dataset without a mask it will take around 1.75 hours. Not bad for a first non-optimized version of the procedures and considering that the free water elimination requires a non-linear convergence step. The free water diffusion tensor derived measures are looking very good. As expected, near to the ventricles and parenchyma, FA values are larger and Diffusion is smaller (Fig1 upper panels) when compared to the values of standard DTI (Fig1 lower panels). There are some voxels with overestimated FA in the ventricles, however these are expected according to Hoy et al., 2014. Users can easily remove these overestimated values of FA by excluding voxels with high free water diffusion volume fractions (Fig.2, idea also mentioned by Hoy et al., 2014. I added the lines of codes that I used to time the free water elimination model and plot the diffusion measures into doc/examples/reconst_fwdti.py which follows a similar structure of the DKI usage example. @etpeterson - I think you can create the example script in the next PR from this script. |
dipy/reconst/fwdti.py
Outdated
| @@ -0,0 +1,608 @@ | |||
| #!/usr/bin/python | |||
| """ Classes and functions for fitting tensors """ | |||
There was a problem hiding this comment.
This top-level docstring seems to be a copy-paste residual
dipy/reconst/tests/test_fwdti.py
Outdated
| assert_almost_equal(FAdti, FAfwe) | ||
| assert_almost_equal(Ffwe, gtf) | ||
|
|
||
| # Test non-linear fit, when no first quess is given |
| pred_sig = np.zeros(f.shape + (gtab.bvals.shape[0],)) | ||
| mask = _positive_evals(evals[..., 0], evals[..., 1], evals[..., 2]) | ||
| index = ndindex(f.shape) | ||
| for v in index: |
There was a problem hiding this comment.
Would it be faster to loop over the indices in np.where(mask) instead?
Now these functions are standalone function and still need to be tested
… in example of usage
before test were almost based only on f values
based on this test a bug was fixed (params were not in the format assumed by function mean_diffusivity)
1 similar comment
doc/examples/reconst_fwdti.py
Outdated
| """ | ||
| Without spatial constrains the free water elimination model cannot be solved | ||
| in data acquired from one non-zero b-value _[Hoy2014]. Therefore, here we | ||
| download a dataset that was required from multi b-values. |
There was a problem hiding this comment.
acquired with multiple b-values
|
This looks very close to be merged. I want to try it with a different dataset too and then will merge (see also comments above). Any other final comments from the rest? |
|
Yup: +1 from me - ready to go as soon as you've had a chance to check things out. @RafaelNH : is there anything we are forgetting, or is this ready to go in from your point-of-view? |
|
Hey guys! Yes for my point of view this is ready to be merge! Please let me know if there are any comments that I forgot to address! Please note that I did some refactoring to use the multi voxel decorator that force me to change some of the features. For example, now when using the non-linear approach this will always use the wls as the first parameters guess. Also S0 is always computed from the mean of the b0 - before I was allowing users to give their own b0 estimate or take it as a model parameter. Because of the multi voxel decorator this is not possible anymore, however I think this is not an issue because the S0 from the mean of the b0 images showed to provide the more robust fits. In general the code is more simplified and easier to read. In addition, I add the f value correction step in the example (the aspect that I discussed above with @samuelstjean). @arokem and @Garyfallidis after testing this please feel free to merge this =). |
1 similar comment
1 similar comment
|
Good job! |
|
Dear, Thanks! |
|
Hello @pppangpang : the implementation in DIPY currently only works for multi-shell data. We do have an implementation of FWDTI with single shell data, based on Marc Golub's work, here: https://github.com/nrdg/fwe. I am currently working to port this to DIPY as well, so it should also be in DIPY soon. |
|
To add to Ariel’s comment: there is a reason why I have not been pushing the single-shell Free-Water DTI implementation into DIPY. As shown in Marc Golub’s work (https://pubmed.ncbi.nlm.nih.gov/33270935/), the single-shell fwDTI model is inherently degenerate. It appears to produce plausible results only because it fixes the tissue mean diffusivity to a constant value. Any deviation from that fixed MD is then interpreted as a change in free water, which can be misleading. We also demonstrated this in a follow-up study (https://pmc.ncbi.nlm.nih.gov/articles/PMC12272235/, where we showed that this constraint led to incorrect conclusions in ageing data, including apparent increases in tissue FA driven solely by the fixed-MD assumption. Despite this, I am glad that @arokem is working on porting the single-shell code to DIPY. However, I would ask that these limitations be clearly stated in the documentation. Free-water changes estimated with single-shell fwDTI can only be considered genuine if there is no reason to suspect alterations in tissue mean diffusivity. Users should be encouraged to explicitly acknowledge this limitation to avoid misleading interpretations. |
|
Yes, I will include these caveats. As you can see in our recent paper, we did find that this approach is helpful in some aspects of tractometry (though to a much lesser degree than the non-degenerate multi-shell approach, if that is possible!), so there are legitimate uses of this model that do not fall into this particular pitfall. Nevertheless, I agree that a stern warning in the documentation about interpreting the FWF as though it is different from MD (e.g., from DTI) would be very appropriate. Your work on this has been very helpful, and I think that the field could use more empirical evaluations of these issues to hammer that message home. Hopefully the availability of the code will help more people make these evaluations. But I am not fooling myself and people will probably also rush to use this in a naive and misguided way. EDIT: fixed a typo. |
In this pull request the free water tensor model will be added to Dipy. The model's fit will be based on the method proposed in http://www.ncbi.nlm.nih.gov/pubmed/25271843.
As @arokem suggested this technique will be implemented in a separate reconst module which I named fwdti.py. Since this technique is an expansion of the simple DTI model, its classes will be defined from inheritance of the classes defined on Dipy's DTI module.
At the moment, I just add the structures for the fwdti class. My very next step will be editing class FreeWaterTensorModel.