-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
MRG Freesurfer coordinate frame tutorial #7578
Conversation
Codecov Report
@@ Coverage Diff @@
## master #7578 +/- ##
==========================================
- Coverage 90.13% 90.13% -0.01%
==========================================
Files 452 452
Lines 82876 82973 +97
Branches 13101 13121 +20
==========================================
+ Hits 74697 74784 +87
- Misses 5350 5356 +6
- Partials 2829 2833 +4 |
@larsoner I've pushed an expansion of the freesurfer tutorial, mostly just adding prose to the explanations without changing the code very substantively. I feel like it ends kind of abruptly, not sure if there's a good way of summarizing / wrapping up / adding some "see also" text? |
I updated the top comment with other stuff I plan to add, WDYT? |
TODO list seems reasonable. I added one item: explanation of how "freesurfer surface RAS" is defined. So far we only say "it's different from nibabel's RAS, and the transform from voxels is called..." without ever saying how it's different. I tried reading the Freesurfer PPT slides that allegedly explain it, but if the answer is in there I was not smart/patient enough to extract it. |
Yes, that has been my experience as well :) I'll have to read and/or talk more to figure that out, but I agree it's worth doing... |
@wmvanvliet @hoechenberger I think I addressed your comments with the latest commit. (Had to force push to deal with a rebase problem) |
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.
Awesome work, @larsoner! No change requests from my side :)
I like the hands on, step by step approach of the rest of the tutorial, but the following sentence had my (slightly sleep deprived) brain run out of processing capacity, even though I know the underlying theory: " These data are voxel intensity values (unsigned integers in the range 0-255). A value data[i, j, k] at a given index triplet (i, j, k) is located at real-world (x, y, z) positions (in RAS orientation) in the scanner’s native coordinate frame (defined during acquisition) by the image’s affine transformation" Trying to parse this I get: Is this interpretation the intended one? If yes, feel free to use that wording, if no, please clarify :) |
Okay addressed comments hopefully, @susamerz can you look again at the description of the affine? I also added the rest of the missing content. @wmvanvliet @hoechenberger want to take another look to see if the new sections make sense to you? @GuillaumeFavelier can you look at the 3D plotting bits and advise how to avoid using a private function? Maybe we need to expose how to get a renderer? Also take a look at the changes I needed to make to the renderer code in general, it was necessary to:
Once the 3D plotting issues are taken care of this will be ready for review/merge. |
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.
some suggestions for clarity/flow
Co-authored-by: Daniel McCloy <[email protected]>
Awesome! |
cool! |
* upstream/master: (21 commits) MRG: Add SSP projectors to Report (mne-tools#7991) BUG: Fix warning (mne-tools#8006) WIP: TFR Doc/test changes (mne-tools#7998) MAINT: Remove numpydoc test on 3.6 (mne-tools#8005) MAINT: Better error message for mismatch (mne-tools#8007) MRG: Allow removal of active projectors if channels they applied to have meanwhile been dropped (mne-tools#8003) MRG Freesurfer coordinate frame tutorial (mne-tools#7578) FIX: Fix stockwell checks (mne-tools#7996) MRG, ENH: Add array-spacing plugin and reorganize deps (mne-tools#7997) MRG, ENH: Reduce memory usage of Welch PSD (mne-tools#7994) STY: One more [ci skip] STY: Docstyle [ci skip] Report parsing (mne-tools#7990) MRG, ENH: BrainVision impedance parsing (mne-tools#7974) BUG: Fix missing source space points (mne-tools#7988) [MRG] Strip base directory name from Report captions when using parse_folder (mne-tools#7986) DOC: Update estimates [skip travis] (mne-tools#7987) DOC: Try to improve find_bad_channels_maxwell doc (mne-tools#7982) VIZ, BUG: fix tickmarks in evoked topomap colorbar (mne-tools#7980) Add low-pass filter to find_bad_channels_maxwell() (mne-tools#7983) ...
The idea is to help people gain fluency with the Freesurfer coordinate frame (since we use it extensively), as well as some stuff about relationships between MRI voxels and affines/MRI coordinate frame. It's not done but at least gets some content out there.
Also makes much nicer some of our info repr stuff (dig, chs entries) by ensuring that on read the constants become "named constants", so that instead of seeing this on
master
:You get:
Todo:
.. warn
to take care of DOC: morph generated from src does not work because fwd exlcude some vertices #7007 (modify themorph
tutorials and errors also?)Source space entries (use_tris, etc.)