add test for ieeg coord_frame read issue #961

[timonmerk]

PR Description

This PR serves as a unit test description for Issue #961. I uploaded a separate "ieeg_bids" dataset in mne_bids\tests\data\ (since I did not find a fitting one on openneuro).

As described in Issue #723 , mne_bids does not correctly read the "iEEGCoordinateSystem" and simply overwrites it with "other".

[welcome]

Hello! 👋 Thanks for opening your first pull request here! ❤️ We will try to get back to you soon. 🚴🏽‍♂️

[JojoVh]

@timonmerk
I looked at his code, and it seems to showcase exactly the issue with the iEEGCoordinateSystem.
Further, I can clarify that the "ieeg_bids" dataset @timonmerk is referring to is a dummy dataset for testing purposes, indeed, i.e. it contains no participant-specific information.

[agramfort]

@timonmerk I pushed here to see the pb. I get:

❯ python mne_bids/tests/test_ieeg_mnt_read.py
/Users/alex/work/src/mne-bids/mne_bids/path.py:1483: RuntimeWarning: Did not find any events.tsv associated with sub-01_ses-01_task-audiovisual_run-01.

The search_str was "mne_bids/tests/data/ieeg_bids/sub-01/**/ieeg/sub-01_ses-01*events.tsv"
  warn(msg)
/Users/alex/work/src/mne-bids/mne_bids/dig.py:495: RuntimeWarning: iEEG Coordinate frame MNI152NLin2009bAsym is not a readable BIDS keyword by mne-bids yet. The allowed keywords are: ['ACPC', 'Pixels', 'Other']
  warn(f"iEEG Coordinate frame {bids_coord_frame} is not a "
/Users/alex/work/src/mne-bids/mne_bids/dig.py:539: RuntimeWarning: Fiducial point nasion not found, assuming identity unknown to head transformation
  raw.set_montage(montage, on_missing='warn')
Traceback (most recent call last):
  File "mne_bids/tests/test_ieeg_mnt_read.py", line 23, in <module>
    test_read_ieeg_bids_data()
  File "mne_bids/tests/test_ieeg_mnt_read.py", line 17, in test_read_ieeg_bids_data
    assert (
AssertionError

you see you have 2 clear warnings. Does it help to address them?

@alexrockhill thoughts?

[alexrockhill]

I left more verbose comments in #961 but in short, I think there is a small PR here to allow passing of the specific template coordinate frame to write_raw_bids and then add support for reading it out in write_raw_bids but this use case is generally supported as documented here: https://mne.tools/mne-bids/dev/auto_examples/convert_ieeg_to_bids.html#step-5-store-coordinates-in-a-template-space. The only tricky thing is that you have to apply_trans to the montage because when you get it from the raw object, if it has fiducials, MNE will automatically store the coordinates in head for you because we try and store all raw coordinates in head (it's a coordinate frame defined with the origin as the center between LPA and RPA which doesn't depend on scan orientation parameters and other stuff that might be confusing so it's not a bad standard coordinate frame). I guess this ended up pretty verbose too :)

[alexrockhill]

Hmmm, I looked into adding support for the standard template coordinate frames (https://bids-specification.readthedocs.io/en/stable/99-appendices/08-coordinate-systems.html#standard-template-identifiers) and the issue is that MNE-Python doesn't have representations for these internally (https://github.com/mne-tools/mne-python/blob/6d0aa2572221c9a710c5965fab96042f3cf9ad3c/mne/transforms.py#L33-L60).

So MNE-BIDS could write out this field with a keyword argument such as MNI152NLin2009bAsym but then when we go to read it back in, the coordinate frame in the raw object (raw_read.get_montage().get_positions()["coord_frame"] in your code) couldn't be MNI152NLin2009bAsym without adding it to the above list of coordinate frames. We could add it to the list but that ends up being a lot of work because, if you support the coordinate frame, you really should be able to do things like convert it to a different coordinate frame and use the template brain etc. In general, allowing people to pass arguments that get written into sidecar files is unideal as well because everything should really be stored in the MNE object.

I think support for the template coordinate frames is something that would be great to add, but it would take a bit of doing. For now, I think the best solution is to have the coordinate frame set to other upon reading and then you know that it's MNI152NLin2009bAsym from reading the dataset sidecar files and treat it accordingly. I think this would be really great to add, but, again, adding support for more coordinate frames/templates/formats takes a bit of doing.

[agramfort]

it's a bit too far from my present needs for me to fully understand what is at stake here. we could discuss this during a next mne dev meeting

…

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

[alexrockhill]

Sounds good re discussing at a the next dev meeting.

From a brief conversation with @larsoner, I think we could quickly add a PR to MNE-Python to make all the BIDS standard templates assignable to the montage but then the issue would be that the raw object couldn't be saved again unless the coordinate frame was converted. This would have to be made very clear that you'd have to define a head->template trans file in order to save to disk. That would just require assigning fiducials. I could put together an example when I have a bit of time.

[alexrockhill]

Ok, this PR should fully solve this issue/do what this PR had started #983, let me know if that works with your test data @timonmerk, I believe it should

[timonmerk]

Hey @alexrockhill, thanks for working so hard on this problem!
I checked out your pull request branch, but when reading the bids run the ieeg_bids data, I receive this warning, and the montage coord_frame is unfortunately still head.

click details:

root=r"C:\Users\ICN_admin\Documents\Decoding_Toolbox\issue_mnebids\mne-bids\mne_bids\tests\data\ieeg_bids"

bids_path = mne_bids.BIDSPath(subject='01', session='01',
task='audiovisual', run='01', root=root)

raw_read = mne_bids.read_raw_bids(bids_path)
Extracting parameters from C:\Users\ICN_admin\Documents\Decoding_Toolbox\issue_mnebids\mne-bids\mne_bids\tests\data\ieeg_bids\sub-01\ses-01\ieeg\sub-01_ses-01_task-audiovisual_run-01_ieeg.vhdr...
Setting channel info structure...
Reading channel info from C:\Users\ICN_admin\Documents\Decoding_Toolbox\issue_mnebids\mne-bids\mne_bids\tests\data\ieeg_bids\sub-01\ses-01\ieeg\sub-01_ses-01_task-audiovisual_run-01_channels.tsv.
Reading in coordinate system frame MNI152NLin2009bAsym: None.
Reading electrode coords from C:\Users\ICN_admin\Documents\Decoding_Toolbox\issue_mnebids\mne-bids\mne_bids\tests\data\ieeg_bids\sub-01\ses-01\ieeg\sub-01_ses-01_space-MNI152NLin2009bAsym_electrodes.tsv.
--- Logging error ---
Traceback (most recent call last):
File "c:\Users\ICN_admin.vscode\extensions\ms-python.python-2022.2.1924087327\pythonFiles\lib\python\debugpy_vendored\pydevd_pydevd_bundle\pydevd_vars.py", line 420, in evaluate_expression
compiled = compile_as_eval(expression)
File "c:\Users\ICN_admin.vscode\extensions\ms-python.python-2022.2.1924087327\pythonFiles\lib\python\debugpy_vendored\pydevd_pydevd_bundle\pydevd_vars.py", line 374, in compile_as_eval
return compile(_expression_to_evaluate(expression), '', 'eval')
File "", line 1
raw_read = mne_bids.read_raw_bids(bids_path)
^
SyntaxError: invalid syntax

During handling of the above exception, another exception occurred:
Traceback (most recent call last):
File "C:\Users\ICN_admin\Anaconda3\envs\mne\lib\logging_init_.py", line 1100, in emit
msg = self.format(record)
File "C:\Users\ICN_admin\Anaconda3\envs\mne\lib\logging_init_.py", line 943, in format
return fmt.format(record)
File "C:\Users\ICN_admin\Anaconda3\envs\mne\lib\logging_init_.py", line 678, in format
record.message = record.getMessage()
File "C:\Users\ICN_admin\Anaconda3\envs\mne\lib\logging_init_.py", line 368, in getMessage
msg = msg % self.args
TypeError: not all arguments converted during string formatting
Call stack:
File "C:\Users\ICN_admin\Anaconda3\envs\mne\lib\runpy.py", line 196, in _run_module_as_main
return _run_code(code, main_globals, None,
File "c:\Users\ICN_admin.vscode\extensions\ms-python.python-2022.2.1924087327\pythonFiles\lib\python\debugpy_vendored\pydevd_pydevd_bundle\pydevd_trace_dispatch_regular.py", line 295, in trace_dispatch_and_unhandled_exceptions
py_db.stop_on_unhandled_exception(py_db, t, additional_info, self._last_exc_arg)
File "c:\Users\ICN_admin.vscode\extensions\ms-python.python-2022.2.1924087327\pythonFiles\lib\python\debugpy_vendored\pydevd_pydevd_bundle\pydevd_breakpoints.py", line 177, in stop_on_unhandled_exception
py_db.do_stop_on_unhandled_exception(thread, user_frame, frames_byid, arg)
File "c:\Users\ICN_admin.vscode\extensions\ms-python.python-2022.2.1924087327\pythonFiles\lib\python\debugpy_vendored\pydevd\pydevd.py", line 2109, in do_stop_on_unhandled_exception
self.do_wait_suspend(thread, frame, 'exception', arg, EXCEPTION_TYPE_UNHANDLED)
File "c:\Users\ICN_admin.vscode\extensions\ms-python.python-2022.2.1924087327\pythonFiles\lib\python\debugpy_vendored\pydevd\pydevd.py", line 1976, in do_wait_suspend
keep_suspended = self._do_wait_suspend(thread, frame, event, arg, suspend_type, from_this_thread, frames_tracker)
File "c:\Users\ICN_admin.vscode\extensions\ms-python.python-2022.2.1924087327\pythonFiles\lib\python\debugpy_vendored\pydevd\pydevd.py", line 2010, in _do_wait_suspend
self.process_internal_commands()
File "c:\Users\ICN_admin.vscode\extensions\ms-python.python-2022.2.1924087327\pythonFiles\lib\python\debugpy_vendored\pydevd\pydevd.py", line 1715, in process_internal_commands
int_cmd.do_it(self)
File "c:\Users\ICN_admin.vscode\extensions\ms-python.python-2022.2.1924087327\pythonFiles\lib\python\debugpy_vendored\pydevd_pydevd_bundle\pydevd_comm.py", line 542, in do_it
self.method(dbg, *self.args, **self.kwargs)
File "c:\Users\ICN_admin.vscode\extensions\ms-python.python-2022.2.1924087327\pythonFiles\lib\python\debugpy_vendored\pydevd_pydevd_bundle\pydevd_constants.py", line 513, in new_func
return func(*args, **kwargs)
File "c:\Users\ICN_admin.vscode\extensions\ms-python.python-2022.2.1924087327\pythonFiles\lib\python\debugpy_vendored\pydevd_pydevd_bundle\pydevd_comm.py", line 1220, in internal_evaluate_expression_json
pydevd_vars.evaluate_expression(py_db, frame, expression, is_exec=True)
File "c:\Users\ICN_admin.vscode\extensions\ms-python.python-2022.2.1924087327\pythonFiles\lib\python\debugpy_vendored\pydevd_pydevd_bundle\pydevd_vars.py", line 359, in new_func
return _run_with_unblock_threads(original_func, py_db, curr_thread, frame, expression, is_exec)
File "c:\Users\ICN_admin.vscode\extensions\ms-python.python-2022.2.1924087327\pythonFiles\lib\python\debugpy_vendored\pydevd_pydevd_bundle\pydevd_vars.py", line 327, in _run_with_unblock_threads
return _run_with_interrupt_thread(original_func, py_db, curr_thread, frame, expression, is_exec)
File "c:\Users\ICN_admin.vscode\extensions\ms-python.python-2022.2.1924087327\pythonFiles\lib\python\debugpy_vendored\pydevd_pydevd_bundle\pydevd_vars.py", line 298, in _run_with_interrupt_thread
return original_func(py_db, frame, expression, is_exec)
File "c:\Users\ICN_admin.vscode\extensions\ms-python.python-2022.2.1924087327\pythonFiles\lib\python\debugpy_vendored\pydevd_pydevd_bundle\pydevd_vars.py", line 422, in evaluate_expression
Exec(_expression_to_evaluate(expression), updated_globals, frame.f_locals)
File "c:\Users\ICN_admin.vscode\extensions\ms-python.python-2022.2.1924087327\pythonFiles\lib\python\debugpy_vendored\pydevd_pydevd_bundle\pydevd_exec2.py", line 3, in Exec
exec(exp, global_vars, local_vars)
File "", line 1, in
File "", line 24, in read_raw_bids
File "C:\Users\ICN_admin\Anaconda3\envs\mne\lib\site-packages\mne_bids\read.py", line 722, in read_raw_bids
_read_dig_bids(electrodes_fname, coordsystem_fname,
File "C:\Users\ICN_admin\Anaconda3\envs\mne\lib\site-packages\mne_bids\dig.py", line 519, in _read_dig_bids
montage = _handle_electrodes_reading(electrodes_fpath, coord_frame,
File "C:\Users\ICN_admin\Anaconda3\envs\mne\lib\site-packages\mne_bids\dig.py", line 42, in _handle_electrodes_reading
logger.info("The read in electrodes file is: \n", summary_str)
Message: 'The read in electrodes file is: \n'
Arguments: ([('name', ['ECOG_0', 'ECOG_1', 'ECOG_2', 'ECOG_3', 'ECOG_4', 'ECOG_5', 'ECOG_6', 'ECOG_7', 'ECOG_8', 'ECOG_9']), ('x', ['22.5', '28.0', '32.0', '37.0', '40.5', '42.5', '45.5', '48.0', '45.5', '48.0']), ('y', ['-38.5', '-29.0', '-19.5', '-10.5', '-2.0', '6.5', '13.5', '22.5', '13.5', '22.5']), ('z', ['83.0', '81.5', '78.0', '73.5', '69.5', '63.0', '55.5', '48.5', '55.5', '48.5']), ('type', ['ECOG', 'ECOG', 'ECOG', 'ECOG', 'ECOG', 'ECOG', 'ECOG', 'ECOG', 'ECOG', 'ECOG'])],)

raw_read.get_montage().get_positions()["coord_frame"]
'head'

[alexrockhill]

Looks like you're not using the new branch, that logger statement was deleted in an earlier PR.

To get the new branch, you can do:

git remote add alex https://github.com/alexrockhill/mne-bids.git
git fetch alex
git checkout alex/templates
pip uninstall mne_bids -y
pip install -e .

I think you might be missing the uninstall mne_bids bit and have an old version that's actually getting imported.

[timonmerk]

Thanks a lot @alexrockhill! I did not know that the "manual" checkout is required to checkout a pull request. I did git fetch origin pull/$ID/head:$BRANCHNAME.

The result for the specific raw_read.get_montage().get_positions()["coord_frame"] is unfortunately unknown, and not MNI152NLin2009bAsym.

[alexrockhill]

So on the issue (mne-tools/mne-python#10405) on the main MNE-Python, we decided that MNE-Python will not have coordinate frame integers for each of the templates because that will give the wrong impression that we fully support those when we don't. I explain it pretty thoroughly at the end of https://6099-89170358-gh.circle-artifacts.com/0/dev/auto_examples/convert_ieeg_to_bids.html, you might want to follow the steps there.

[sappelhoff]

@timonmerk could you please:

1. ensure you have no uncommitted changes in your working directory (git status)
2. git checkout main
3. git fetch
4. git reset --hard origin/main
5. git checkout -b bug/ieeg/coords
6. git push -u origin bug/ieeg/coords
7. open a new PR from your new "bug/ieeg/coords" branch
8. close this PR, cross linking to the new one in the GitHub web UI
9. git checkout main
10. git fetch upstream main
11. git reset --hard upstream/main
12. git push --force

The last steps assume you have a git remote "upstream" -- check git remote -v to see that you do. If you don't, add it via: git remote add upstream <X>, where <X> is either the https or ssh address of this repo (mne-tools/mne-bids), depending on your workflow.

EDIT: Alternatively, you can invite me as a collaborator to your repo, and I'll do these steps myself :-)

[timonmerk]

New Pull Request: #1051

[timonmerk]

@sappelhoff, thanks for those detailed instructions. I assume you wanted to change the branch name of the pull request through those steps?
Also I assumed from Alex previous comment that the support of additional coordinate frames is not planned anymore, but nice to hear that the issue is still relevant

[sappelhoff]

Yes, PRs should never be submitted from your main branch :) it'll screw up your workflow if you ever want to create more than one branch at a time.

Practically you having this PR on your main branch meant that I got an email notification whenever the ci on your repo ran according to schedule (one a day). And I couldn't figure out how to change this manually. So two birds with one stone: stop the emails, and fix your workflow. :)

Thanks for doing this. And yes, the issue is still relevant imho.

[timonmerk]

Ahh ok, thanks @sappelhoff. That makes sense!
I just want to mention that the last step unfortunately failed on my machine:

(base)
ICN_admin@DESKTOP-1DMNS5J MINGW64 /c/code/mne_bids_pr/mne-bids (main)
$ git remote -v
origin  https://github.com/timonmerk/mne-bids.git (fetch)
origin  https://github.com/timonmerk/mne-bids.git (push)
upstream        https://github.com/mne-tools/mne-bids.git (fetch)
upstream        https://github.com/mne-tools/mne-bids.git (push)
(base)
ICN_admin@DESKTOP-1DMNS5J MINGW64 /c/code/mne_bids_pr/mne-bids (main)
$ git reset --hard upstream/main
fatal: ambiguous argument 'upstream/main': unknown revision or path not in the working tree.
Use '--' to separate paths from revisions, like this:
'git <command> [<revision>...] -- [<file>...]'

You can see that I added the upstream link, but the reset failed for some reason..

[alexrockhill]

Also I assumed from Alex previous comment that the support of additional coordinate frames is not planned anymore, but nice to hear that the issue is still relevant

I'd love to improve that but I couldn't get any traction in the BIDS community making the changes needed over there to make it make any sense by the time it gets to mne. There's issues with licensing the T1 images if BIDS were to repost them is the main thing. I think it's a bit of a moot point anyway because I would recommend sharing in the patients' coordinate frame and not template so that data users can choose their own template (with a defaced T1).

[sappelhoff]

You can see that I added the upstream link, but the reset failed for some reason..

But step 4 worked, right?

Perhaps you need to run git fetch upstream main first, before resetting your main branch to be like the upstream one 🤔.

If that doesn't work, try git pull upstream main (this might lead to an error, which would be fine) and then the reset command.

[sappelhoff]

@timonmerk please let me know once you tried this and whether it works!

as of now, your main branch is still out of sync: https://github.com/timonmerk/mne-bids

[timonmerk]

@sappelhoff Now it worked, thanks!

ICN_admin@DESKTOP-1DMNS5J MINGW64 /c/code/mne_bids_pr/mne-bids (main)
$ git reset --hard upstream/main
HEAD is now at 6ed57f5d try fix for docs (nirs example) (#1050)

[sappelhoff]

Perfect, now you need to git push --force to get these changes to your origin :-)

[timonmerk]

Ahh, forgot that last step. Now it should be up to date.
I definitely need to reiterate good practices for git usage :)