MRG: Add OPM example#5525
Conversation
Codecov Report
@@ Coverage Diff @@
## master #5525 +/- ##
==========================================
+ Coverage 88.3% 88.31% +<.01%
==========================================
Files 361 363 +2
Lines 67406 67456 +50
Branches 11421 11427 +6
==========================================
+ Hits 59521 59571 +50
- Misses 5043 5044 +1
+ Partials 2842 2841 -1 |
|
Ready for review/merge from my end. @ rzetter @ jiivana @LauriParkkonen can you take a look at the rendered example and make sure I didn't break anything: https://9416-1301584-gh.circle-artifacts.com/0/html/auto_examples/datasets/plot_opm_data.html Also @rzetter please check |
|
Looks great!
Some minor remarks:
- low-pass filter at 40 Hz is rather low for responses to electric nerve stimulation (the sharp N20 response to median nerve stimulation is less visible) but maybe there is too much 50-Hz interference for opening the passband to e.g. 90 Hz. Rasmus can comment on this as he’s been playing with the filter.
- I would display less (use higher threshold) for the MNE on brain surface – now it’s a bit hard to see where’s the peak
Eric Larson <notifications@github.com<mailto:notifications@github.com>> kirjoitti 19.9.2018 kello 22.46:
Ready for review/merge from my end.
@ rzetter @ jiivana @LauriParkkonen<https://github.com/LauriParkkonen> can you take a look at the rendered example and make sure I didn't break anything:
https://9416-1301584-gh.circle-artifacts.com/0/html/auto_examples/datasets/plot_opm_data.html
Also @rzetter<https://github.com/rzetter> please check whats_new.rst and datasets_index.rst to make sure you're happy with the URL I chose and credits, etc. Let me know if there are other names (w/URLs) I should add.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub<#5525 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/ABPxbLmdmi69uNofcYUDJU33ZqUycXg3ks5ucp8RgaJpZM4Wplsk>.
|
|
Hi, this looks great! I have some minor comments: -The low-pass can and should be upped to e.g. 90 Hz, the 40 Hz limit is something that might have been left from my earlier scripts by mistake. A slight high-pass could also be used (0.03 Hz or 0.1 Hz), but I guess it isn't needed. However, a 50 Hz notch filter will be useful, since we typically do have some (electrical) 50 Hz noise in the OPMs. -Agreed with Lauri regarding the color map limits in the MNE visualization. With the current limits, the entire physiologically interesting region is saturated with white. Also, the color bar in the current figure seems to have been corrupted somehow. -Some information about the experiment paradigm could be added to the start of the script plot_opm_data.py, e.g.: -I also sent Neuromag SQUID data for the same experiment, do you want to include that in the example for comparison purposes? -Great that you've added a mechanism to use custom coil definitions on the fly, that will definitely be useful! -Also great to see the tweaks to the 3D visualization, we already had to do some small hacks to our local MNE installations for proper visualization in plot_alignment(). in examples/datasets/plot_opm_data.py, lines 96-98: to areas with low/no sensitivity occurs. Constraining the source space toareas we are sensitive to might be a good idea."Clumsy sentence (by me), remove 'occur'. in datasets_index.rst, lines 231-232: With these small tweaks, everything should be good for review/merge. I'll leave it up to you if you want to include SQUID data as a comparison or not. |
|
@rzetter to make life easier in the future if you click over to the "Files changed" tab, you can leave comments directly in line with the code. I'll try to address comments in the next couple of days |
| """Get the sensor shape vertices.""" | ||
| rrs = np.empty([0, 2]) | ||
| tris = np.empty([0, 3], int) | ||
| from scipy.spatial import ConvexHull |
There was a problem hiding this comment.
what is the convention in placing imports inside the functions?
There was a problem hiding this comment.
Some scipy imports are nested to improve import mne speed. There is a test for violations in mne/tests/test_import_nesting.py::test_module_nesting and the rules are listed there
| def test_plot_alignment(tmpdir): | ||
| """Test plotting of -trans.fif files and MEG sensor layouts.""" | ||
| # generate fiducials file for testing | ||
| tempdir = _TempDir() |
There was a problem hiding this comment.
I changed it to be tempdir = str(tmpdir). This works around a pytest bug where tmpdir on older pytest is not seen properly as a string by some functions, so casting it explicitly to a string makes some things work better
| info_cube['chs'][0]['coil_type'] = 9999 | ||
| with pytest.raises(RuntimeError, match='coil definition not found'): | ||
| plot_alignment(info_cube, meg='sensors', surfaces=()) | ||
| coil_def_fname = op.join(str(tmpdir), 'temp') |
There was a problem hiding this comment.
If you are using pytest tmpdir I would do this:
coil_def_fname = tmpdir.join('temp')
| # ----------------------------- | ||
| # First we filter and epoch the data: | ||
|
|
||
| raw = mne.io.Raw(raw_fname, preload=True) |
|
beautiful ! thanks @larsoner @LauriParkkonen and @rzetter for making this happen. |
5 participants
Todo:
whats_new.rst