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

DTI uncertainty visualization #810

Merged
merged 14 commits into from Feb 27, 2024
Merged

Conversation

tvcastillod
Copy link
Contributor

Hi, this PR aims to give the possibility to visualize the uncertainty in the DTI model.

Because the DTI visualization pipeline is quite complex, a level of uncertainty arises, which if visualized, could help to assess the accuracy of the model. The selected method is based on first-order matrix perturbation analysis and is described here. The idea is to examine the uncertainty in the eigenvalues and eigenvectors, to estimate and visualize the variance of the main direction of diffusion, which is represented with symmetrical cones, allowing for the visualization of the diffusion direction and confidence interval simultaneously.

Other sources include 1 and 2, which give a slightly more detailed description of the uncertainty calculation.

  1. Variance of estimated DTI-derived parameters via first-order perturbation methods [see appendix B]
  2. Theoretical analysis of the effects of noise on diffusion tensor imaging [see appendix D]

@skoudoro
Copy link
Contributor

That is an exciting feature !!! Please, let us know when it is ready for reviews/questions

@tvcastillod tvcastillod changed the title [WIP] DTI uncertainty visualization DTI uncertainty visualization Jul 10, 2023
@tvcastillod
Copy link
Contributor Author

@skoudoro @guaje I made the changes that were discussed, now you can do a first review.

Copy link
Contributor

@JoaoDell JoaoDell left a comment

Choose a reason for hiding this comment

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

Hey @tvcastillod, just finished my first review on your PR. I have ran the tests and everything seems fine, test_uncertainty.py outputted the results you have already showed so it is working. I noticed some issues and have some questions so I will wait for you to comment on that further.

fury/actors/tensor.py Show resolved Hide resolved
fury/actors/tensor.py Show resolved Hide resolved
fury/actors/tensor.py Outdated Show resolved Hide resolved
fury/actors/tensor.py Show resolved Hide resolved
fury/actors/tensor.py Show resolved Hide resolved
fury/actors/tests/test_uncertainty.py Outdated Show resolved Hide resolved
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This script is just for you to see how this new feature can be used. It shows the display of the diffusion tensor ellipsoids, and then their associated uncertainty. dipy is needed to load and read the data.

254139234-e78def39-a4d3-4c3d-8dff-4e6c14dfe171

Copy link
Contributor

@ganimtron-10 ganimtron-10 left a comment

Choose a reason for hiding this comment

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

Hey @tvcastillod ,
Giving a first look everything looks good.
Will tryout and give some feedback.

fury/actors/tensor.py Outdated Show resolved Hide resolved
Copy link
Contributor

@skoudoro skoudoro left a comment

Choose a reason for hiding this comment

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

Overall, it looks good,

I still need to test it locally a bit deeper. See below my first comment.
thnks @tvcastillod

fury/actor.py Outdated Show resolved Hide resolved
fury/actors/tensor.py Show resolved Hide resolved
fury/actors/tensor.py Outdated Show resolved Hide resolved
@skoudoro
Copy link
Contributor

Also, tests are failing with this new function, Please, Can you look deeper what is going on ? we have a segfault in one of your shader

@codecov
Copy link

codecov bot commented Aug 7, 2023

Codecov Report

Merging #810 (d555495) into master (2bba1b9) will increase coverage by 0.20%.
Report is 184 commits behind head on master.
The diff coverage is 92.77%.

❗ Current head d555495 differs from pull request most recent head 022a73b. Consider uploading reports for the commit 022a73b to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #810      +/-   ##
==========================================
+ Coverage   84.33%   84.54%   +0.20%     
==========================================
  Files          44       44              
  Lines       10356    10529     +173     
  Branches     1410     1418       +8     
==========================================
+ Hits         8734     8902     +168     
- Misses       1252     1255       +3     
- Partials      370      372       +2     
Files Coverage Δ
fury/actor.py 85.60% <87.50%> (+0.01%) ⬆️
fury/actors/tensor.py 96.39% <94.02%> (-3.61%) ⬇️

... and 4 files with indirect coverage changes

@skoudoro
Copy link
Contributor

skoudoro commented Aug 8, 2023

Hi @tvcastillod,
What is the status concerning this PR and other reviews ? thank you for the feedback

@tvcastillod
Copy link
Contributor Author

@guaje @skoudoro I addressed all the comments, you can take another look to verify that everything is correct.

Copy link
Contributor

@guaje guaje left a comment

Choose a reason for hiding this comment

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

Other than these small comments, this PR looks good to me.

fury/shaders/sdf/sd_cone.frag Outdated Show resolved Hide resolved
fury/shaders/sdf/sd_union.frag Outdated Show resolved Hide resolved
fury/actors/tensor.py Show resolved Hide resolved
@pep8speaks
Copy link

Hello @tvcastillod, Thank you for updating!

Line 425:74: W605 invalid escape sequence '\D'
Line 426:49: W605 invalid escape sequence '\T'
Line 427:40: W605 invalid escape sequence '\e'
Line 427:51: W605 invalid escape sequence '\D'
Line 427:57: W605 invalid escape sequence '\e'
Line 428:34: W605 invalid escape sequence '\e'
Line 432:9: W605 invalid escape sequence '\T'
Line 432:25: W605 invalid escape sequence '|'
Line 432:27: W605 invalid escape sequence '\D'
Line 432:33: W605 invalid escape sequence '\e'
Line 432:43: W605 invalid escape sequence '|'
Line 471:22: E127 continuation line over-indented for visual indent

To test for issues locally, pip install flake8 and then run flake8 fury.

Copy link
Contributor

@skoudoro skoudoro left a comment

Choose a reason for hiding this comment

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

Thank you for this @tvcastillod,

I am going ahead and merge this PR.

@skoudoro skoudoro merged commit 74fd1bc into fury-gl:master Feb 27, 2024
2 of 28 checks passed
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

6 participants