Skip to content

NF: Free water tensor model#835

Merged
Garyfallidis merged 111 commits intodipy:masterfrom
RafaelNH:free_water_DTI
Sep 12, 2016
Merged

NF: Free water tensor model#835
Garyfallidis merged 111 commits intodipy:masterfrom
RafaelNH:free_water_DTI

Conversation

@RafaelNH
Copy link
Copy Markdown
Contributor

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.

@etpeterson
Copy link
Copy Markdown
Contributor

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.

@etpeterson
Copy link
Copy Markdown
Contributor

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.

@RafaelNH
Copy link
Copy Markdown
Contributor Author

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.

@RafaelNH
Copy link
Copy Markdown
Contributor Author

PS: I will be working on this in the following 2 hours.

@arokem
Copy link
Copy Markdown
Contributor

arokem commented Jan 20, 2016

I'll just add for the benefit of both of you that you can also use our
gitter channel to coordinate: https://gitter.im/nipy/dipy

I've found it useful for coordinating work with others when working
simultaneously on the same thing. And I'd love to stay in the loop on this
work, so it will also allow me to see what's going on :-)

On Wed, Jan 20, 2016 at 2:34 AM, Rafael Neto Henriques <
[email protected]> wrote:

PS: I will be working on this in the following 2 hours.


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

@RafaelNH
Copy link
Copy Markdown
Contributor Author

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:

  • Find bug in WLS that is causing fwdti to fail
  • Define the properties of the class FreeWaterTensorFit (that is easy)
  • Add the model's prediction function (analogous to the dti and dki modules)
  • Prediction function have to be tested.
  • Implementing the NLS part of the articles procedure
  • Finding the Jacobian for the NLS fit

@etpeterson
Copy link
Copy Markdown
Contributor

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.

@RafaelNH
Copy link
Copy Markdown
Contributor Author

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@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.

@etpeterson
Copy link
Copy Markdown
Contributor

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

elinimation -> elimination

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

thanks!

@Garyfallidis
Copy link
Copy Markdown
Contributor

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.

dipy/reconst/fwdti.py                                183     38     36      8    75%   123-127, 134-136, 153-156, 161, 171-172, 462-485, 544, 581-587, 120->128, 133->134, 148->153, 158->161, 167->171, 459->462, 539->542, 543->544

@Garyfallidis
Copy link
Copy Markdown
Contributor

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 ?

@etpeterson
Copy link
Copy Markdown
Contributor

That works for me @Garyfallidis

@RafaelNH
Copy link
Copy Markdown
Contributor Author

RafaelNH commented Feb 9, 2016

@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?

@Garyfallidis
Copy link
Copy Markdown
Contributor

Sounds awesome! Fly high! :)

@RafaelNH
Copy link
Copy Markdown
Contributor Author

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.

@@ -0,0 +1,608 @@
#!/usr/bin/python
""" Classes and functions for fitting tensors """
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This top-level docstring seems to be a copy-paste residual

@RafaelNH RafaelNH changed the title (WIP) NF: Free water tensor model NF: Free water tensor model Feb 11, 2016
assert_almost_equal(FAdti, FAfwe)
assert_almost_equal(Ffwe, gtf)

# Test non-linear fit, when no first quess is given
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Typo: "quess" => "guess"

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:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Would it be faster to loop over the indices in np.where(mask) instead?

@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage increased (+0.06%) to 83.081% when pulling 8a40caf on RafaelNH:free_water_DTI into 31bc6fe on nipy:master.

1 similar comment
@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage increased (+0.06%) to 83.081% when pulling 8a40caf on RafaelNH:free_water_DTI into 31bc6fe on nipy:master.

"""
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.
Copy link
Copy Markdown
Contributor

@Garyfallidis Garyfallidis Sep 11, 2016

Choose a reason for hiding this comment

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

acquired with multiple b-values

@Garyfallidis
Copy link
Copy Markdown
Contributor

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?

@arokem
Copy link
Copy Markdown
Contributor

arokem commented Sep 11, 2016

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?

@RafaelNH
Copy link
Copy Markdown
Contributor Author

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 =).

@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage increased (+0.06%) to 83.085% when pulling 31661d1 on RafaelNH:free_water_DTI into 31bc6fe on nipy:master.

1 similar comment
@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage increased (+0.06%) to 83.085% when pulling 31661d1 on RafaelNH:free_water_DTI into 31bc6fe on nipy:master.

@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage increased (+0.06%) to 83.085% when pulling 31661d1 on RafaelNH:free_water_DTI into 31bc6fe on nipy:master.

1 similar comment
@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage increased (+0.06%) to 83.085% when pulling 31661d1 on RafaelNH:free_water_DTI into 31bc6fe on nipy:master.

@Garyfallidis
Copy link
Copy Markdown
Contributor

Good job!

@pppangpang
Copy link
Copy Markdown

Dear,
I am using the "dipy.reconst.fwdti" for the calculation of free-water elimination, I want to confirm that the dMRI should be multi-shell data (at least two non-zero b-values), or single-shell (e.g., b=0 and b=1000) is fine as from the paper https://pubmed.ncbi.nlm.nih.gov/19623619/

Thanks!

@arokem
Copy link
Copy Markdown
Contributor

arokem commented Nov 19, 2025

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.

@RafaelNH
Copy link
Copy Markdown
Contributor Author

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.

@arokem
Copy link
Copy Markdown
Contributor

arokem commented Nov 19, 2025

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.

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.

8 participants