[MRG] Datatype handling when in write_raw_bids #774

richardkoehler · 2021-04-16T11:09:44Z

PR Description

closes #752

The user must now specify the datatype when calling write_raw_bids. Not automatic datatype inferring is done anymore in write_raw_bids. However, it is checked if the specified datatype is present in the dataset.

Merge checklist

Maintainer, please confirm the following before merging:

The user must now specify the datatype when calling write_raw_bids. Not automatic datatype inferring is done anymore in write_raw_bids. However, it is checked if the specified datatype is present in the dataset.

…e-bids into write_EEG_iEEG

data type specifciation by user is only required if multiple data types are present in data

agramfort

just a few suggestions.

thx @richardkoehler

agramfort · 2021-04-17T12:21:10Z

mne_bids/utils.py

+    if 'eeg' in raw:
+        datatypes.append('eeg')
+    if len(datatypes) == 0:
+        raise ValueError('No MEG, EEG or iEEG channels found in data.'


Suggested change

raise ValueError('No MEG, EEG or iEEG channels found in data.'

raise ValueError('No MEG, EEG or iEEG channels found in data. '

mne_bids/utils.py

agramfort · 2021-04-17T12:26:48Z

mne_bids/utils.py

+        if datatype in raw:
+            datatype_matches = True
+    elif datatype == 'meg':
+        if datatype not in raw:


agramfort · 2021-04-17T12:30:13Z

mne_bids/write.py

+    if bids_path.datatype is None:
+        datatype = _handle_datatype(raw)
+    else:
+        datatype = bids_path.datatype
+        _check_datatype(raw, datatype)


I would do this in the _handle_datatype function

datatype = _handle_datatype(raw, bids_path.datatype)

then you call _check_datatype in _handle_datatype and your return directly the 2nd parameter if valid.

mne_bids/utils.py

Co-authored-by: Alexandre Gramfort <alexandre.gramfort@m4x.org>

richardkoehler · 2021-04-18T16:16:10Z

@agramfort Thanks for the helpful suggestions! Tests are currently failing because write_raw_bids() now makes no more automatic assumptions about the datatype if there are multiple ephys data types (meg, eeg or ieeg) in our Raw object and because it actually checks if the bids_path.datatype is present in the Raw object. I can start fixing the tests, but as I already wrote in #752, I think the current PR might "break" write_raw_bids() for a lot of existing code out there (as demonstrated by the failing tests). So I think we need to make sure that everyone agrees with the new behavior of write_raw_bids:

If bids_path.datatype is not set, _handle_datatype will try to infer the datatype:
a) If there is a single data type (of meg, eeg, ieeg) in the Raw object, this type will be picked
b) If there are multiple types (of meg, eeg, ieeg) in the Raw object, an Error will be thrown, asking the user to specify the data type
c) If no data type (of meg, eeg, ieeg) is found in the Raw object, an Error will be thrown, asking the user to specify the channel types and/or bids_path.datatype
If bids_path.datatype is set, _handle_datatype will call _check_datatype and verify that the specified data type is actually present in the Raw object. If it is not, an Error will be thrown.

Alternatively, we could "trust" the user to know what he is doing, and simply issue a warning message for case 2. There might be some cases where a user might want to write the data without specifying the channel types, even though I can't currently think of any. Is it really the job of MNE-BIDS to enforce these kind of things?
@jasmainak

agramfort · 2021-04-19T06:29:26Z

I would say that if you have no datatype and the data contains MEG and EEG then you should infer automatically it's an meg data. would this already avoid breaking many tests?

richardkoehler · 2021-04-19T12:10:56Z

@agramfort Yes, this actually fixed many of the tests. I implemented this way of handling datatype=None and fixed the remaining failing tests. In the case of datatype='eeg' with additional 'meg' channels in the data we are now trusting the user to know what he is doing, and NOT enforcing the datatype 'meg'. Therefore, the user can decide to write out combined MEG-EEG recordings in an EEG data format.

richardkoehler · 2021-04-19T12:22:13Z

@agramfort Actually, it won't be possible as I get the following Error:

E           UserWarning: Encountered unsupported non-voltage units: T/m, T
E           Note that the BrainVision format specification supports only µV.

So apparently it doesn't make any sense to save MEG-EEG data as EEG data because brainvision doesn't support MEG units. Sorry, I'm not very familiar with MEG, I wasn't aware of this.

mne_bids/utils.py

agramfort · 2021-04-19T15:57:56Z

mne_bids/tests/test_utils.py

+    datatype = None
+    with pytest.raises(ValueError, match=f'The specified datatype {datatype} '
+                                         'is currently not supported.'):
+        _check_datatype(raw_eeg, datatype)
+    datatype = 'anat'
+    with pytest.raises(ValueError, match=f'The specified datatype {datatype} '
+                                         'is currently not supported.'):
+        _check_datatype(raw_eeg, datatype)
+    datatype = 'eeg'
+    _check_datatype(raw_eeg, datatype)
+    with pytest.raises(ValueError, match=f'The specified datatype {datatype} '
+                                         'was not found in the raw object.'):
+        _check_datatype(raw_meg, datatype)


Suggested change

datatype = None

with pytest.raises(ValueError, match=f'The specified datatype {datatype} '

'is currently not supported.'):

_check_datatype(raw_eeg, datatype)

datatype = 'anat'

with pytest.raises(ValueError, match=f'The specified datatype {datatype} '

'is currently not supported.'):

_check_datatype(raw_eeg, datatype)

datatype = 'eeg'

_check_datatype(raw_eeg, datatype)

with pytest.raises(ValueError, match=f'The specified datatype {datatype} '

'was not found in the raw object.'):

_check_datatype(raw_meg, datatype)

for datatype, raw in [(None, raw_eeg, ('eeg', raw_meg), ('anat', raw_eeg)]:

with pytest.raises(ValueError, match=f'The specified datatype {datatype} '

'is currently not supported.'):

_check_datatype(raw, datatype)

can you compress with something like this?

I did, hope it is fine the way I have solved it now :)

Co-authored-by: Alexandre Gramfort <alexandre.gramfort@m4x.org>

codecov-commenter · 2021-04-19T17:02:41Z

Codecov Report

Merging #774 (503046f) into main (bc488fc) will increase coverage by 0.06%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #774      +/-   ##
==========================================
+ Coverage   94.01%   94.07%   +0.06%     
==========================================
  Files          23       23              
  Lines        2822     2851      +29     
==========================================
+ Hits         2653     2682      +29     
  Misses        169      169

Impacted Files	Coverage Δ
mne_bids/utils.py	`96.51% <100.00%> (+0.58%)`	⬆️
mne_bids/write.py	`97.31% <100.00%> (ø)`

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 bc488fc...503046f. Read the comment docs.

agramfort

thx @richardkoehler !

agramfort · 2021-04-20T06:47:21Z

thx a lot @richardkoehler !

richardkoehler · 2021-04-20T06:57:24Z

@agramfort Thanks for the merge, happy to contribute :) Stupid question because this is my first contribute to mne-bids: The Merge checklist at the top is not for myself to complete right? Because it says "This is not your own PR". I haven't updated whats_new.rst or anything, but of course I would be happy to do so, if required

agramfort · 2021-04-20T07:28:05Z

it's for you to fill to help us review but here I looked myself. thx again

…

hoechenberger · 2021-04-20T08:10:21Z

Thanks @richardkoehler!

hoechenberger · 2021-04-20T10:16:38Z

Oh noes, we forgot to add a changelog entry!! @richardkoehler care to add one?

richardkoehler · 2021-04-20T10:53:54Z

@hoechenberger Yes, I did so in #779 as I couldn't reopen this PR.

richardkoehler added 6 commits April 16, 2021 13:06

Datatype handling when in write_raw_bids

02b3035

The user must now specify the datatype when calling write_raw_bids. Not automatic datatype inferring is done anymore in write_raw_bids. However, it is checked if the specified datatype is present in the dataset.

Merge branch 'main' into write_EEG_iEEG

a16944b

Small bug and style fixe

dc9a61e

Merge branch 'write_EEG_iEEG' of https://github.com/richardkoehler/mn…

d74eeed

…e-bids into write_EEG_iEEG

Fix Error

94930ce

data type specifciation by user is only required if multiple data types are present in data

datatype inferring for meg restored

81d98ca

richardkoehler mentioned this pull request Apr 16, 2021

Write out combined EEG and iEEG recordings #752

Closed

agramfort reviewed Apr 17, 2021

View reviewed changes

richardkoehler commented Apr 18, 2021

View reviewed changes

mne_bids/utils.py Outdated Show resolved Hide resolved

richardkoehler and others added 4 commits April 18, 2021 16:16

Move datatype check into _handle_datatype

c0e9650

Apply suggestions from code review

23a1de7

Co-authored-by: Alexandre Gramfort <alexandre.gramfort@m4x.org>

PEP fix

bcd1c2b

Fix test_handle_datatype

8f50a1a

richardkoehler added 2 commits April 19, 2021 08:51

restore data type inferring for multiple types

1634ecf

Update tests

6be15ab

Fix handling of combined MEG-i/EEG data

a6d73b9

agramfort reviewed Apr 19, 2021

View reviewed changes

richardkoehler and others added 2 commits April 19, 2021 18:13

Apply suggestions from code review

3729f79

Co-authored-by: Alexandre Gramfort <alexandre.gramfort@m4x.org>

Review

503046f

agramfort approved these changes Apr 20, 2021

View reviewed changes

agramfort merged commit 154f44b into mne-tools:main Apr 20, 2021

richardkoehler changed the title ~~Datatype handling when in write_raw_bids~~ [MRG] Datatype handling when in write_raw_bids Apr 20, 2021

richardkoehler deleted the write_EEG_iEEG branch April 20, 2021 06:58

richardkoehler mentioned this pull request Apr 20, 2021

Update changelog for closed PR #774 #779

Merged

6 tasks

hoechenberger pushed a commit that referenced this pull request Apr 20, 2021

Update changelog for closed PR #774 (#779)

577bc4c

christinerogers mentioned this pull request Jun 22, 2021

Release 0.8 ? #816

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MRG] Datatype handling when in write_raw_bids #774

[MRG] Datatype handling when in write_raw_bids #774

richardkoehler commented Apr 16, 2021 •

edited

agramfort left a comment

agramfort Apr 17, 2021

agramfort Apr 17, 2021

agramfort Apr 17, 2021

richardkoehler commented Apr 18, 2021

agramfort commented Apr 19, 2021 via email

richardkoehler commented Apr 19, 2021

richardkoehler commented Apr 19, 2021

agramfort Apr 19, 2021

richardkoehler Apr 19, 2021

codecov-commenter commented Apr 19, 2021 •

edited

agramfort left a comment

agramfort commented Apr 20, 2021

richardkoehler commented Apr 20, 2021

agramfort commented Apr 20, 2021 via email

hoechenberger commented Apr 20, 2021

hoechenberger commented Apr 20, 2021

richardkoehler commented Apr 20, 2021

	raise ValueError('No MEG, EEG or iEEG channels found in data.'
	raise ValueError('No MEG, EEG or iEEG channels found in data. '

[MRG] Datatype handling when in write_raw_bids #774

[MRG] Datatype handling when in write_raw_bids #774

Conversation

richardkoehler commented Apr 16, 2021 • edited

PR Description

Merge checklist

agramfort left a comment

Choose a reason for hiding this comment

agramfort Apr 17, 2021

Choose a reason for hiding this comment

agramfort Apr 17, 2021

Choose a reason for hiding this comment

agramfort Apr 17, 2021

Choose a reason for hiding this comment

richardkoehler commented Apr 18, 2021

agramfort commented Apr 19, 2021 via email

richardkoehler commented Apr 19, 2021

richardkoehler commented Apr 19, 2021

agramfort Apr 19, 2021

Choose a reason for hiding this comment

richardkoehler Apr 19, 2021

Choose a reason for hiding this comment

codecov-commenter commented Apr 19, 2021 • edited

Codecov Report

agramfort left a comment

Choose a reason for hiding this comment

agramfort commented Apr 20, 2021

richardkoehler commented Apr 20, 2021

agramfort commented Apr 20, 2021 via email

hoechenberger commented Apr 20, 2021

hoechenberger commented Apr 20, 2021

richardkoehler commented Apr 20, 2021

richardkoehler commented Apr 16, 2021 •

edited

codecov-commenter commented Apr 19, 2021 •

edited