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

Feat: Implement a visualisation feature to NiTransforms #212

Open
wants to merge 58 commits into
base: master
Choose a base branch
from

Conversation

jmarabotto
Copy link
Contributor

Addition of new file vis.py. Calls nitransform's nonlinear DenseFieldTransform class to illustrate corresponding transform objects through:

  • Distorted image and grid
  • Quiver plot coloured according to Diffusion Scalar map
  • Jacobian map

which can be illustrated all together (3x3 grid of plots, showing axial, coronal and sagittal projections for each graph) or individually.

@jmarabotto jmarabotto marked this pull request as draft July 10, 2024 09:55
@effigies
Copy link
Member

Can you post screenshots?

Julien Marabotto and others added 28 commits July 11, 2024 14:53
Creation of new directory vim containing scripts for plotting
…draft) interactive slider feature to switch between slices.
…n up straneous comments and code; implement NotImplemented errors for sliders
@jmarabotto
Copy link
Contributor Author

Can you post screenshots?

Overview (3x3 plot grid):
https://output.circle-artifacts.com/output/job/bb500396-f329-4c10-b994-6ac6365bc443/artifacts/0/tmp/tests/artifacts/show_transform.svg
NOTE sliders don't work yet but would be a cool interactive tool for updating slices.

Distorted grid lines superimposed over deformed brain density map:
https://output.circle-artifacts.com/output/job/bb500396-f329-4c10-b994-6ac6365bc443/artifacts/0/tmp/tests/artifacts/plot_distortion.svg

Quiver plot of the deltas field:
https://output.circle-artifacts.com/output/job/bb500396-f329-4c10-b994-6ac6365bc443/artifacts/0/tmp/tests/artifacts/plot_quiverdsm.svg
NOTE colours represent the diffusion scalar map of the vector field (red: dominant x-component; green: dominant y-component; blue: dominant z-component; light->dark: weak dominance -> strong dominance)

Jacobian map:
https://output.circle-artifacts.com/output/job/bb500396-f329-4c10-b994-6ac6365bc443/artifacts/0/tmp/tests/artifacts/plot_jacobian.svg

Hopefully this sets a good (or useful) base for the implementation of a visualisation module to nitransforms, or at least can act as a good starting point to spring new ideas.

Copy link

codecov bot commented Jul 11, 2024

Codecov Report

Attention: Patch coverage is 96.25850% with 11 lines in your changes missing coverage. Please review.

Project coverage is 94.71%. Comparing base (a366f85) to head (36dae68).
Report is 32 commits behind head on master.

Files with missing lines Patch % Lines
nitransforms/vis.py 96.56% 4 Missing and 6 partials ⚠️
nitransforms/io/fsl.py 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #212      +/-   ##
==========================================
+ Coverage   94.39%   94.71%   +0.31%     
==========================================
  Files          15       16       +1     
  Lines        1713     2004     +291     
  Branches      323      374      +51     
==========================================
+ Hits         1617     1898     +281     
- Misses         79       83       +4     
- Partials       17       23       +6     
Flag Coverage Δ
unittests 94.71% <96.25%> (+0.31%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jmarabotto jmarabotto marked this pull request as ready for review July 15, 2024 12:53
@jmarabotto
Copy link
Contributor Author

@effigies this is now ready for review! Latest plots can be accessed through the CircleCi workflow, under build_pytest > Artifacts. These shouldn't be too different (if at all) to the links posted above.

Copy link
Member

@effigies effigies left a comment

Choose a reason for hiding this comment

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

I'd be more comfortable with some of the numeric code if it came with tests; e.g., comparing the construction of the Jacobian determinant image with that generated by ANTs' CreateJacobianDeterminantImage. Given that this is visualization, there is less concern that we have an error (that will presumably show up in a visualization) and no other numeric code depends on this.

I would be okay merging as-is, but I left some small comments.

nitransforms/tests/test_vis.py Outdated Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

The final cell seems to have failed:

image

Copy link
Contributor Author

@jmarabotto jmarabotto Jul 23, 2024

Choose a reason for hiding this comment

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

I've worked more on the notebook and fixed this since the last commit, it simply requires a 3x3 subplot (I carelessly copy pasted a 1x3 figure)

Edit: committed to fix, should work now 👍

nitransforms/tests/test_vis.py Outdated Show resolved Hide resolved
nitransforms/vis.py Outdated Show resolved Hide resolved
Comment on lines +46 to +59
try:
"""if field provided by path"""
self._voxel_size = nb.load(transform).header.get_zooms()[:3]
assert len(self._voxel_size) == 3
except TypeError:
"""if field provided by numpy array (eg tests)"""
deltas = []
for i in range(self._xfm.ndim):
deltas.append((np.max(self._xfm._field[i]) - np.min(self._xfm._field[i]))
/ len(self._xfm._field[i]))

assert np.all(deltas == deltas[0])
assert len(deltas) == 3
self._voxel_size = deltas
Copy link
Member

Choose a reason for hiding this comment

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

It would be better not to rely on different code paths for tests and public API, if at all possible.

Can the test be rewritten to save a NIfTI image into tmp_path and pass that to PlotDenseField()?

nitransforms/vis.py Outdated Show resolved Hide resolved
nitransforms/vis.py Outdated Show resolved Hide resolved
jmarabotto and others added 8 commits July 23, 2024 11:11
Co-authored-by: Chris Markiewicz <effigies@gmail.com>
Co-authored-by: Chris Markiewicz <effigies@gmail.com>
Co-authored-by: Chris Markiewicz <effigies@gmail.com>
Co-authored-by: Chris Markiewicz <effigies@gmail.com>
Co-authored-by: Chris Markiewicz <effigies@gmail.com>
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.

2 participants