-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
[BUG] Fix mne.viz.Brain.add_volume_labels matrix ordering bug #11730
Conversation
This works for me now
|
mne/viz/_brain/_brain.py
Outdated
ornt = nib.orientations.axcodes2ornt( | ||
nib.orientations.aff2axcodes(aseg.affine) | ||
).astype(int) | ||
ras_ornt = nib.orientations.axcodes2ornt("RAS") | ||
ornt_trans = nib.orientations.ornt_transform(ornt, ras_ornt) | ||
aseg_data = nib.orientations.apply_orientation(aseg_data, ornt_trans) | ||
aff_trans = nib.orientations.inv_ornt_aff(ornt_trans, aseg.shape) | ||
vox_mri_t = np.dot(vox_mri_t, aff_trans) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am confused why this is necessary -- the vox_mri_t
should already take care of putting everything in MRI surface RAS coordinates for us. Also, there are a number of places we use vox_mri_t
and don't use any axcodes2ornt
stuff, so it's weird this is the one place we need to do it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... to me it suggests maybe there is some bug with how _marching_cubes
works or something, rather than some adjustment we need to make to vox_mri_t
.
I would investigate deeper by using freeview
and some specific voxel i, j, k
in some known subvolume and what their viewer says for MRI surface RAS x, y, z
, confirm that this is just vox_mri_t @ (i, j, k, 1)
(it had better be!), then inspect the outputs at various points of this function for that label. It should tell us where the problem is...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So it looks like it was caused by the fortran order check in _marching_cubes
. I think the solution I just pushed works, here is one issue where the order needed to be fortran #10860 which lead to the order being auto-detected with "A"
. Checking to be sure that that works now too.
Huh so oddly, in the tumor case, the |
Both of these work now using the tumor.nrrd file from here: #10860. It's not ideal that it's opposite of what you'd expect but I'm not really sure if there's a workaround that's better other than just documenting it with a verbose comment. |
Okay, after much debugging, part of the issue was with |
Oh also I in doing this I read into the
|
Thanks @alexrockhill ! |
The lastest dev version of MNE correctly plots. 👍👍👍 Thanks a lot !!! |
* upstream/main: (24 commits) Allow int-like as ID of make_fixed_length_events (mne-tools#11748) Easycap-M43 montage (mne-tools#11744) ENH: Create a Calibrations class for eyetracking data (mne-tools#11719) Fix alphabetical order in overview/people.rst, fix sphinx formatting in docstrings and set verbose to keyword-only (mne-tools#11745) Add Mathieu Scheltienne to MNE-Python Steering Council (mne-tools#11741) removed requirement for curv.*h files to create Brain object (mne-tools#11704) [BUG] Fix mne.viz.Brain.add_volume_labels matrix ordering bug (mne-tools#11730) Fix installer links (mne-tools#11729) MAINT: Update for PyVista deprecation (mne-tools#11727) MAINT: Update roadmap (mne-tools#11724) MAINT: Update download link [skip azp] [skip cirrus] [skip actions] fix case for chpi_info[1] == None (mne-tools#11714) Add cmap argument for mne.viz.utils.plot_sensors (mne-tools#11720) BUG: Fix one more PySide6 bug (mne-tools#11723) MAINT: Fix PySide6 and PyVista compat (mne-tools#11721) MRG: If _check_fname() cannot find a file, display the path in quotation marks to help spot accidental trailing spaces (mne-tools#11718) Add "array-like" to `_validate_type()` (mne-tools#11713) MAINT: Avoid problematic PySide6 (mne-tools#11715) Fix installer links (mne-tools#11709) Updating change log after PR mne-tools#11575 (mne-tools#11707) ...
Fixes #11728