-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
MRG : source reconstruction on EEG using fsaverage brain #6146
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
Conversation
…ing fsaverage brain
|
we should put here a link to the forward tutorial rather than duplicating
the narrative. I won't touch at this before the sprint next week
… |
|
it's WIP so we don't loose this. If you have time to improve please
push directly to this branch.
|
|
Circle errors on the example: Traceback (most recent call last):
File "/home/circleci/.local/lib/python3.7/site-packages/sphinx_gallery/gen_rst.py", line 398, in _memory_usage
multiprocess=True)
File "/home/circleci/.local/lib/python3.7/site-packages/memory_profiler.py", line 336, in memory_usage
returned = f(*args, **kw)
File "/home/circleci/.local/lib/python3.7/site-packages/sphinx_gallery/gen_rst.py", line 389, in __call__
exec(self.code, self.globals)
File "/home/circleci/project/tutorials/plot_eeg_no_mri.py", line 58, in <module>
subjects_dir=subjects_dir, trans=trans_fname)
File "</home/circleci/project/mne/externals/decorator.py:decorator-gen-117>", line 2, in plot_alignment
File "/home/circleci/project/mne/utils/_logging.py", line 89, in wrapper
return function(*args, **kwargs)
File "/home/circleci/project/mne/viz/_3d.py", line 815, in plot_alignment
% (subject, '\n'.join(try_fnames)))
OSError: No head surface found for subject fsaverage after trying:
/home/circleci/mne_data/MNE-sample-data/subjects/fsaverage/bem/outer_skin.surf
/home/circleci/mne_data/MNE-sample-data/subjects/fsaverage/bem/flash/outer_skin.surf
/home/circleci/mne_data/MNE-sample-data/subjects/fsaverage/bem/fsaverage-head.fif |
|
@agramfort Sure, I can try, but I had a lot of problems before trying to push to other people's PRs. If you also have a tutorial on that, that would be helpful. :) |
|
Regarding the error: is it possible that mne's fsaverage ships without head surface? I remember I had to construct it when using fsaverage for source localization. |
|
yes a new fsaverage dataset is necessary or at least an update of sample
dataset to add these missing surfaces. I had to compute them locally.
… |
|
If you upload the surfaces I can update the dataset. Or if it's as simple as |
|
yes it's just running mne watershed_bem
we can add this to sample but I think it could be smart to have a tiny
dataset with just fsaverage
for those we want to do EEG with MRI templates
|
|
We already have |
|
+1 for a specific fetcher like mne.datasets.fetch_fsaverage_3layer_bem
but I would not assume that sample is present to have it working
… |
|
This tuto does something similar to what we have in #5836 but with |
Codecov Report
@@ Coverage Diff @@
## master #6146 +/- ##
==========================================
+ Coverage 87.21% 87.21% +<.01%
==========================================
Files 412 412
Lines 73985 73985
Branches 12270 12270
==========================================
+ Hits 64523 64525 +2
+ Misses 6592 6588 -4
- Partials 2870 2872 +2 |
|
The current solution requires to download 1.8Gb we could either allow for:
|
|
@agramfort for now, putting it in the root of |
| # check all montages | ||
| # | ||
|
|
||
| fix_units = {'EGI_256':'cm', 'GSN-HydroCel-128':'cm', 'GSN-HydroCel-129':'cm', |
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.
EGI_256 is not in cm, take a look at this comment: the distance for each channel from the center is exactly 1. for EGI_256.
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 missed your comment. The way you figured out the distance is smarter than the way I did it.
The code was pulled from an early stage of #5836 and yes EGI_256 it's broken. Now that we are at it, when load EGI_256 with eeg=['original', 'projected'] you can see that the original do have a 'brainish' shape but the projected does not.
Either way, we decided with @agramfort to add the example ploting all helmets i a different PR. (I'll ping you there)
|
Maybe just mne.datasets.fetch_fsaverage(subjects_dir=None,
update_path=True), and update_path=True will set the path using the
set_montage... function. And we can even then make the set_montage...
thing private.
I like that. Make it private and make sure fetch_fsaverage works with a
good default path without thinking about it.
I prefer the fetch_ nomenclature here instead of data_path since it does
something closer to what the existing fetch_ functions do than to what
the existing data_path functions do.
I had proposed to use eegbci because it's edf files which are standard for
EEG people. It feels more natural.
Above you said "We should use sample here"
<#6146 (comment)>
which is why I switched to the sample dataset. EEGBCI is fine with me,
though showing a field map is a particularly uninteresting example, and
something users are unlikely to want to do. It would be much better to,
say, compute some classifier and then project it to source space or
something. I can do it at the sprint next week.
my bad. I meant "not use sample" ie just use an fsaverage template dataset.
I suggested to do sensitivity maps to display something. Then the example
should not document the full pipeline but point to the narrative on making
epochs and inverse operators. This is to avoid a scattered doc that is
harder to maintain.
|
|
I'm okay with the sensitivity map, we should just link at the end to a standard MNE tutorial or something then |
|
yes +100
|
|
Yes. Lets move on
|
|
Feel free to push to my branch the commit to go back to eegbci |
mne/datasets/_fsaverage.py
Outdated
| return subjects_dir | ||
|
|
||
|
|
||
| def set_montage_coreg_path(subjects_dir=None): |
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 would make this private
agramfort
left a comment
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.
@massich I am lost reviewing this PR. I see unrelated things and I still see the use of set_montage_coreg_path
mne/bem.py
Outdated
| if incomplete == 'raise': | ||
| raise RuntimeError(msg) | ||
| else: | ||
| warn(msg) |
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.
why is this here???
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.
got here likely by accident in 8d1023e
mne/commands/tests/test_commands.py
Outdated
| fnames = glob.glob(op.join(tempdir, '*proj.fif')) | ||
| assert len(fnames) == 1 | ||
| fnames = glob.glob(op.join(tempdir, '*-eve.fif')) | ||
| assert len(fnames) == 1 |
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.
how did this happen in this PR?
mne/commands/mne_watershed_bem.py
Outdated
| help="Change the preflood height", default=None) | ||
| parser.add_option("--copy", dest="copy", | ||
| help="Use copies instead of symlinks for surfaces", | ||
| action="store_true") |
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.
@larsoner all this bem related things that were added to the PR. Were they needed?? I mean, they are unrelated to the PR. But maybe you wanted them.
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.
They were needed when I created the 3-layer BEM for fsaverage, so yes please keep
| subjects_dir = op.join(sample.data_path(download=False), 'subjects') | ||
| # Copy necessary files to tempdir | ||
| tempdir = str(tmpdir) | ||
| tempdir = _TempDir() |
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.
We want almost everything from this commit. Feel free to push a new PR but we shouldn't lose them
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.
no prob. I'll get it back and just ping you to make sure I did not forget anything important.
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.
Actually I made a PR #6192 feel free to push in it.
|
Didn't see this ... very cool MNE is making an effort to support the "lazy and stupid" crowd finally!!! |
drammock
left a comment
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 reviewed only the Tutorial at @massich's request; there are lots of small improvements that are possible, but nothing that is unclear enough to be a blocker.
tutorials/plot_eeg_no_mri.py
Outdated
| # The files live in: | ||
| subject = 'fsaverage' | ||
| trans = op.join(fs_dir, 'bem', 'fsaverage-trans.fif') | ||
| src = op.join(fs_dir, 'bem', 'fsaverage-5-src.fif') |
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 think this needs to be 'fsaverage-ico-5-src.fif?
larsoner
left a comment
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.
LGTM +1 for merge. Rendered example:
https://12457-1301584-gh.circle-artifacts.com/0/dev/auto_tutorials/plot_eeg_no_mri.html
|
the failing error is unrelated. Merging |
this PR aims to cover a long standing issue with MNE which is to do EEG source reconstruction using a template brain here fsaverage.
Limitation of the current PR:
mne.datasets.fetch_fsaverage_3layer_bemor so@drammock any thoughts on how to integrate this in the new in the making documentation?
@massich this is a good test for the new digitization refactoring