FIX: Segmentation plots aligned with cardinal axes #544
Conversation
Codecov Report
@@ Coverage Diff @@
## master #544 +/- ##
==========================================
- Coverage 64.63% 59.60% -5.03%
==========================================
Files 44 43 -1
Lines 5378 5382 +4
Branches 786 785 -1
==========================================
- Hits 3476 3208 -268
- Misses 1744 2036 +292
+ Partials 158 138 -20
Continue to review full report at Codecov.
|
oesteban
force-pushed
the
fix/542-rotate-to-cardinal
branch
2 times, most recently
from
4380b9e to
8208368
Compare
|
Before: After: (there should not be any difference here because all our tests are on images aligned with cardinal axes, I know the reports look good for fMRIPrep with oblique datasets). |
niworkflows/viz/utils.py
Outdated
| image_nii = as_canonical(_3d_in_file(image_nii)) | ||
| seg_niis = [as_canonical(_3d_in_file(f)) for f in seg_niis] |
There was a problem hiding this comment.
These are guaranteed to have the same affines? If not, the implicit rotation by removing the rotational components of the affine my produce a misalignment.
Might be worth having an affine check before calling as_canonical.
There was a problem hiding this comment.
In principle, right now it should allow you to stick in a NIfTI clipping some area (e.g., a face mask given the circumstances) with different voxel size and FoV. Not sure we are ever using this.
What if we run as_canonical only once and apply the same rotation matrix to all other inputs?
niworkflows/viz/utils.py
Outdated
| bbox_nii = ( | ||
| image_nii if bbox_nii is None | ||
| else as_canonical(_3d_in_file(bbox_nii)) | ||
| ) |
There was a problem hiding this comment.
I'm not entirely sure what bbox is supposed to be, so it's hard to say if reorienting it can be potentially problematic.
There was a problem hiding this comment.
bbox is a mask from which the visible extent of the plot will be calculated. Typically, you pass a brain mask and the mosaic will be zoomed in into the bounding box of that brain mask (thus, making sure you don't render blank planes).
There was a problem hiding this comment.
I'm pretty much always uncomfortable with deleting affine information
You're not alone. It seemed safe enough to me as the NIfTI with new affine information is not saved tho hard disk (and it can't possibly cascade). We could think of some ways of indicating the user that the affine information has been modified for the purpose of visualization.
I feel like my approach would be to pad the images with zeroes so the bounding box is outside the plottable area.
How would this fix the issue? I'm under the impression that the problem comes from running marching cubes on 2D planes where there is not a full voxel intersecting. I haven't checked the influence of the data extents, but when there are enough intersecting voxels it seems nilearn deals just fine regardless of the bounding box.
niworkflows/viz/utils.py
Outdated
| image_nii = as_canonical(_3d_in_file(image_nii)) | ||
| seg_niis = [as_canonical(_3d_in_file(f)) for f in seg_niis] |
There was a problem hiding this comment.
In principle, right now it should allow you to stick in a NIfTI clipping some area (e.g., a face mask given the circumstances) with different voxel size and FoV. Not sure we are ever using this.
What if we run as_canonical only once and apply the same rotation matrix to all other inputs?
niworkflows/viz/utils.py
Outdated
| bbox_nii = ( | ||
| image_nii if bbox_nii is None | ||
| else as_canonical(_3d_in_file(bbox_nii)) | ||
| ) |
There was a problem hiding this comment.
bbox is a mask from which the visible extent of the plot will be calculated. Typically, you pass a brain mask and the mosaic will be zoomed in into the bounding box of that brain mask (thus, making sure you don't render blank planes).
|
I think an alternative approach would be to densely resample the masks so that you make sure some voxel center will fall close enough of the cutting plane so that the marching cubes algorithm doesn't trip. This seemed too heuristic to me, as there's no easy way to calculate the upsampling factor you need to ensure the contour extraction will work. Dropping the rotation, however, does ensure the intersection of visualization planes will not be oblique. In addition, this deobliquing for visualization would be also useful, e.g., to plot SDC. |
|
Best reviewed: commit by commit
Optimal code review plan (1 warning)
|
oesteban
force-pushed
the
fix/542-rotate-to-cardinal
branch
from
c7894ef to
7cdab20
Compare
While nilearn/nilearn#25 is considered, this PR advances the generation of mosaics after aligning the affine matrix of the data with the cardinal axes. This should overcome the glitches when nilearn is requested to plot contours and the mask for the ROI has voxels that only partially intersect with the visualization plane. Resolves: nipreps#542. References: nipreps#281, nipreps#304. Depends: nipreps#543.
oesteban
force-pushed
the
fix/542-rotate-to-cardinal
branch
from
7cdab20 to
f11d73c
Compare
There was a problem hiding this comment.
Since the original NIfTI affine is preserved in the outputs, I don't see a huge problem with altering it in the context of a plot, though perhaps I'm missing something?
Plotting issues have become more exposed when testing nipreps/fmriprep#2130, so this / a similar fix are gonna be essential before a release.
Some of these concerns are being discussed in nilearn/nilearn#2525 - although, while I see them crucial for nilearn (as a general-purpose library), I don't think they make any difference in the context of niworkflows. |
|
Okay, I think I finally properly understand the situation, and that what we want is to see the data array, with properly scaled voxels and orientation information, but the plotter should not be given obliquity (rotation and shear) information that would cause it to rotate the voxels relative to the pixels in the display grid. Assuming this is right, I will check the code from this perspective. |
|
Question: is it the case that the two images should always have the same affine/FoV? I would assume so, but it doesn't seem to be documented. |
Correct
If "display grid" = the current plane being plotted, yes this is correct.
The two images should have the same rotation w.r.t. the cardinal axes. In theory, they could have different zooms because the rotation is extracted from the reference and then applied to the masks. However, I don't know (as you said, it is not documented anywhere) whether nilearn imposes having the same affine. Either way, this PR only requires the rotation w.r.t. cardinal to be the same. |
There was a problem hiding this comment.
Apologies for taking so long on this. I find it hard to think through the logic when we're changing the world space. I think this works, and I'm pretty sure we're using it on images that have the same affine/FoV, so it would be hard to mess up. I'm not 100% that it will work even when affines/FoV don't match.
Two minor fixes.
Co-authored-by: Chris Markiewicz <[email protected]>
|
@oesteban Do you want to rerun the test? I can't see any reason it should suddenly start failing. |
|
Tests pass locally. Merging. |
First release in the 1.3.x series. This release includes enhancements and bug-fixes towards the release of the first LTS version of fMRIPrep. PyBIDS has been revised to use more recent versions, a series of ANTs' interfaces have been deemed ready to upstream into Nipype, and several improvements regarding multi-echo EPI are included. With thanks to Basile Pinsard for contributions. * FIX: Patch ``ApplyTransforms`` spec to permit identity in a chain (#554) * FIX: Add dots to extensions in PyBIDS' config file (#548) * FIX: Segmentation plots aligned with cardinal axes (#544) * FIX: Skip T1w file existence check if ``anat_derivatives`` are provided (#545) * FIX: Avoid diverting CIFTI dtype from original BOLD (#532) * ENH: Add ``smooth`` input to ``RegridToZooms`` (#549) * ENH: Enable ``DerivativesDataSink`` to take multiple source files to derive entities (#547) * ENH: Allow ``bold_reference_wf`` to accept multiple EPIs/SBRefs (#408) * ENH: Numerical stability of EPI brain-masks using probabilistic prior (#485) * ENH: Add a pure-Python interface to resample to specific resolutions (#511) * MAINT: Finalize upstreaming of ANTs' interfaces to Nipype (#550) * MAINT: Update to Python +3.6 (#541)
1.3.0rc3 First release in the 1.3.x series. This release includes enhancements and bug-fixes towards the release of the first LTS version of fMRIPrep. PyBIDS has been revised to use more recent versions, a series of ANTs' interfaces have been deemed ready to upstream into Nipype, and several improvements regarding multi-echo EPI are included. With thanks to Basile Pinsard for contributions. * FIX: Patch ``ApplyTransforms`` spec to permit identity in a chain (nipreps#554) * FIX: Add dots to extensions in PyBIDS' config file (nipreps#548) * FIX: Segmentation plots aligned with cardinal axes (nipreps#544) * FIX: Skip T1w file existence check if ``anat_derivatives`` are provided (nipreps#545) * FIX: Avoid diverting CIFTI dtype from original BOLD (nipreps#532) * ENH: Add ``smooth`` input to ``RegridToZooms`` (nipreps#549) * ENH: Enable ``DerivativesDataSink`` to take multiple source files to derive entities (nipreps#547) * ENH: Allow ``bold_reference_wf`` to accept multiple EPIs/SBRefs (nipreps#408) * ENH: Numerical stability of EPI brain-masks using probabilistic prior (nipreps#485) * ENH: Add a pure-Python interface to resample to specific resolutions (nipreps#511) * MAINT: Finalize upstreaming of ANTs' interfaces to Nipype (nipreps#550) * MAINT: Update to Python +3.6 (nipreps#541)
1.3.0 First release in the 1.3.x series. This release includes enhancements and bug-fixes towards the release of the first LTS (*long-term support*) version of *fMRIPrep*. *PyBIDS* has been revised to use more recent versions, a series of ANTs' interfaces have been deemed ready to upstream into *Nipype*, and several improvements regarding multi-echo EPI are included. With thanks to Basile Pinsard for contributions. * FIX: Patch ``ApplyTransforms`` spec to permit identity in a chain (nipreps#554) * FIX: Add dots to extensions in PyBIDS' config file (nipreps#548) * FIX: Segmentation plots aligned with cardinal axes (nipreps#544) * FIX: Skip T1w file existence check if ``anat_derivatives`` are provided (nipreps#545) * FIX: Avoid diverting CIFTI dtype from original BOLD (nipreps#532) * ENH: Add ``smooth`` input to ``RegridToZooms`` (nipreps#549) * ENH: Enable ``DerivativesDataSink`` to take multiple source files to derive entities (nipreps#547) * ENH: Allow ``bold_reference_wf`` to accept multiple EPIs/SBRefs (nipreps#408) * ENH: Numerical stability of EPI brain-masks using probabilistic prior (nipreps#485) * ENH: Add a pure-Python interface to resample to specific resolutions (nipreps#511) * MAINT: Upstream all bug-fixes in the 1.2.9 release * MAINT: Finalize upstreaming of ANTs' interfaces to Nipype (nipreps#550) * MAINT: Update to Python +3.6 (nipreps#541)
While nilearn/nilearn#2525 is considered, this PR advances the generation of mosaics after aligning the affine matrix of the data with the cardinal axes. This should overcome the glitches when nilearn is requested to plot contours and the mask for the ROI has voxels that only partially intersect with the visualization plane.
Resolves: #542.
References: #281, #304.
Depends: #543.