[ENH, MRG] Allow template coordinate frame writing #980

alexrockhill · 2022-03-07T20:27:26Z

Fixes last part from #973

Merge checklist

Maintainer, please confirm the following before merging:

alexrockhill · 2022-03-07T21:24:09Z

@adam2392 these should all come back green, I think this would be nice to have in the release tomorrow but they should be reviewed. Do you have some time today to do a bit more thorough review. I've tested and it should be good but ideally it wouldn't break anything on the stable version!

codecov · 2022-03-07T21:27:41Z

Codecov Report

Merging #980 (c29942d) into main (7343e20) will increase coverage by 0.27%.
The diff coverage is 98.43%.

@@            Coverage Diff             @@
##             main     #980      +/-   ##
==========================================
+ Coverage   94.72%   94.99%   +0.27%     
==========================================
  Files          25       25              
  Lines        3698     3678      -20     
==========================================
- Hits         3503     3494       -9     
+ Misses        195      184      -11

Impacted Files	Coverage Δ
mne_bids/write.py	`96.75% <88.88%> (-0.08%)`	⬇️
mne_bids/config.py	`97.59% <100.00%> (+0.02%)`	⬆️
mne_bids/dig.py	`97.58% <100.00%> (+4.78%)`	⬆️
mne_bids/utils.py	`96.09% <100.00%> (-0.02%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 33cbe67...c29942d. Read the comment docs.

doc/whats_new.rst

examples/convert_ieeg_to_bids.py

hoechenberger · 2022-03-07T21:23:38Z

mne_bids/config.py

@@ -289,7 +285,7 @@
 MNE_FRAME_TO_STR = {val: key for key, val in MNE_STR_TO_FRAME.items()}

 # see BIDS specification for description we copied over from each
-BIDS_COORD_FRAME_DESCRIPTIONS = {
+BIDS_COORD_FRAME_DESCRIPTIONS = dict(**{


uh there's gotta be a better way to do this. In the worst case, simply first create a dict and then append to it / update it, please.

hoechenberger · 2022-03-07T21:26:03Z

mne_bids/dig.py

+        with warnings.catch_warnings():
+            warnings.filterwarnings(action='ignore', category=RuntimeWarning,
+                                    message='.*nasion not found', module='mne')
+            raw.set_montage(montage)


Have you considered setting the verbosity level of set_montage() to 'error'? (I didn't take a look at this method, not sure if it even supports verbosity!)

There could be other warnings though... wouldn't want to miss those

mne_bids/dig.py

hoechenberger · 2022-03-07T21:30:21Z

mne_bids/dig.py

+    with warnings.catch_warnings():
+        warnings.filterwarnings(action='ignore', category=RuntimeWarning,
+                                message='.*nasion not found', module='mne')
+        raw.set_montage(montage, on_missing='warn')


see above: can you maybe change the verbosity level?

mne_bids/tests/test_dig.py

hoechenberger · 2022-03-07T21:32:52Z

mne_bids/tests/test_read.py

-    with pytest.warns(RuntimeWarning, match="Skipping EEG electrodes.tsv"):
+    with pytest.raises(RuntimeError, match="'head' coordinate frame "


Wait, what? So this is a breaking behavior change? If yes, we might need a deprecation cycle for this?

I don't think there is anything to cycle, it's raising an error where before there was a warning. I think this is appropriate and needed; I really don't like when among all the things that are printed out you get a warning that the channels weren't written which is kind of a big deal if you think they are supposed to be. This is a pretty edge case anyway, when you have a montage in head with no fiducials.

hoechenberger · 2022-03-07T21:36:40Z

@alexrockhill We can also postpone the release to Wednesday or so, so you have more time to get this here cleaned up & tested

alexrockhill · 2022-03-07T21:46:42Z

@alexrockhill We can also postpone the release to Wednesday or so, so you have more time to get this here cleaned up & tested

Okay, trying to respond to all comments and test right away not to hold things up

alexrockhill · 2022-03-07T23:51:40Z

There was one part of the coverage I wasn't able to get to: when orient == 'n/a' which is when MEG isn't in one of the recognized formats. According to BIDS, you should be able to write MEG in any of the EEG formats, edf for instance, but we don't currently allow that. The behavior would be that it would write with the given BIDSPath.space coordinate frame if one was provided, if not the behavior wouldn't change.

alexrockhill · 2022-03-08T00:01:20Z

Ah that was an annoying bug, I modified the raw when passing it to write_raw_bids, this should be good to go now. Sorry this is a bit later in the day East Coast.

alexrockhill · 2022-03-08T02:28:38Z

Ok to merge @hoechenberger and @adam2392 ?

agramfort

looks good https://6077-89170358-gh.circle-artifacts.com/0/dev/auto_examples/convert_ieeg_to_bids.html#step-6-store-coordinates-in-another-template-space-accepted-by-bids

but I realize we don't support pyvista and 3D plots in the doc of mne-bids... to be done later I guess

hoechenberger · 2022-03-08T08:21:21Z

Thanks, @alexrockhill!

alexrockhill · 2022-03-08T12:43:03Z

looks good https://6077-89170358-gh.circle-artifacts.com/0/dev/auto_examples/convert_ieeg_to_bids.html#step-6-store-coordinates-in-another-template-space-accepted-by-bids

but I realize we don't support pyvista and 3D plots in the doc of mne-bids... to be done later I guess

Those are block quotes, not actual executed code since we don't have that template in the sample data. I wouldn't doubt that the 3D scraper is also not implemented but they shouldn't render.

agramfort · 2022-03-08T13:20:28Z

arfff should we add this in mne-bids to make sure this code does not rot?

…

Message ID: ***@***.***>

alexrockhill · 2022-03-08T13:28:24Z

Sure, actually while I was writing my response, I thought that fsaverage is actually a template that we do have so we could use that and make it executable. It would also be a lot longer because you'd have to convert the coordinates to voxel and scanner RAS. I'll make a PR and we can see if it looks reasonable.

alexrockhill · 2022-03-08T15:30:09Z

There were a few issues with this PR but I'm almost done fixing and rewriting the example (I think the mne.viz.Brain would be nice but isn't needed). I will make another PR ASAP.

alexrockhill added 4 commits March 7, 2022 11:15

wip

258fbae

allow template coordinate frames for write

24e4dd1

fix flake

d1203fc

fix tests

36b0be9

hoechenberger reviewed Mar 7, 2022

View reviewed changes

richard review, fix codecov

d173e75

fix raw file modified issue

c29942d

agramfort approved these changes Mar 8, 2022

View reviewed changes

hoechenberger mentioned this pull request Mar 8, 2022

Add 3D figure rendering to documentation #981

Closed

hoechenberger merged commit 5798e49 into mne-tools:main Mar 8, 2022

alexrockhill deleted the ieeg branch March 8, 2022 13:28

alexrockhill mentioned this pull request Mar 8, 2022

[BUG, MRG] Fix coordinate frames #982

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ENH, MRG] Allow template coordinate frame writing #980

[ENH, MRG] Allow template coordinate frame writing #980

alexrockhill commented Mar 7, 2022 •

edited

Loading

alexrockhill commented Mar 7, 2022

codecov bot commented Mar 7, 2022 •

edited

Loading

hoechenberger Mar 7, 2022

hoechenberger Mar 7, 2022

alexrockhill Mar 7, 2022

hoechenberger Mar 7, 2022

hoechenberger Mar 7, 2022

alexrockhill Mar 8, 2022

hoechenberger commented Mar 7, 2022

alexrockhill commented Mar 7, 2022

alexrockhill commented Mar 7, 2022

alexrockhill commented Mar 8, 2022

alexrockhill commented Mar 8, 2022

agramfort left a comment

hoechenberger commented Mar 8, 2022

alexrockhill commented Mar 8, 2022

agramfort commented Mar 8, 2022 via email

alexrockhill commented Mar 8, 2022

alexrockhill commented Mar 8, 2022

		with pytest.warns(RuntimeWarning, match="Skipping EEG electrodes.tsv"):
		with pytest.raises(RuntimeError, match="'head' coordinate frame "

[ENH, MRG] Allow template coordinate frame writing #980

[ENH, MRG] Allow template coordinate frame writing #980

Conversation

alexrockhill commented Mar 7, 2022 • edited Loading

Merge checklist

alexrockhill commented Mar 7, 2022

codecov bot commented Mar 7, 2022 • edited Loading

Codecov Report

hoechenberger Mar 7, 2022

Choose a reason for hiding this comment

hoechenberger Mar 7, 2022

Choose a reason for hiding this comment

alexrockhill Mar 7, 2022

Choose a reason for hiding this comment

hoechenberger Mar 7, 2022

Choose a reason for hiding this comment

hoechenberger Mar 7, 2022

Choose a reason for hiding this comment

alexrockhill Mar 8, 2022

Choose a reason for hiding this comment

hoechenberger commented Mar 7, 2022

alexrockhill commented Mar 7, 2022

alexrockhill commented Mar 7, 2022

alexrockhill commented Mar 8, 2022

alexrockhill commented Mar 8, 2022

agramfort left a comment

Choose a reason for hiding this comment

hoechenberger commented Mar 8, 2022

alexrockhill commented Mar 8, 2022

agramfort commented Mar 8, 2022 via email

alexrockhill commented Mar 8, 2022

alexrockhill commented Mar 8, 2022

alexrockhill commented Mar 7, 2022 •

edited

Loading

codecov bot commented Mar 7, 2022 •

edited

Loading