Make ch_type reading more robust & support RESP #482

hoechenberger · 2020-07-21T16:05:27Z

PR Description

If mapping of channel types from BIDS to MNE fails, retry with all-uppercase channel types. Closes GH-481.
I anticipate this will only be a temporary thing and therefore didn't add a dedicated test for this. – However, it does come with a breaking change: we're not using a defaultdict for the mapping anymore.

This commit also adds support for mapping respiratory channels between BIDS and MNE.

Merge checklist

Maintainer, please confirm the following before merging:

All comments resolved
This is not your own PR
All CIs are happy
PR title starts with [MRG]
whats_new.rst is updated
PR description includes phrase "closes <#issue-number>"
Commit history does not contain any merge commits

If mapping of channel types from BIDS to MNE fails, retry with all-uppercase channel types. Closes mne-toolsGH-481. I anticipate this will only be a temporary thing and therefore didn't add a dedicated test for this. This commit also adds support for mapping respiratory channels between BIDS and MNE.

codecov-commenter · 2020-07-21T17:09:32Z

Codecov Report

Merging #482 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #482   +/-   ##
=======================================
  Coverage   93.70%   93.70%           
=======================================
  Files          11       11           
  Lines        1762     1763    +1     
=======================================
+ Hits         1651     1652    +1     
  Misses        111      111

Impacted Files	Coverage Δ
mne_bids/read.py	`95.40% <100.00%> (+0.08%)`	⬆️
mne_bids/utils.py	`95.66% <100.00%> (-0.04%)`	⬇️

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 e2798b3...81dd026. Read the comment docs.

mne_bids/tests/test_write.py

jasmainak · 2020-07-21T18:07:07Z

Why did you remove defaultdict?

hoechenberger · 2020-07-21T18:47:52Z

@jasmainak

Why did you remove defaultdict?

I thought we had discussed that in #481 in the top comment, but actually I only posted a followup comment and talked about this with @agramfort on Slack.

From #481:

The ds001810 dataset, however, uses lowercase channel types, and hence read_raw_bids() assigns the type misc to all of them (and, later, somehow, they all beome eeg, but I'm not sure yet where this occurs).

Quoting my followup comment:

Forgot to add:
I'm not sure using a defaultdict() is the best idea here:

mne-bids/mne_bids/utils.py

Lines 460 to 464 in e2798b3

# Make it a defaultdict to prevent key errors

ch_type_mapping = defaultdict(lambda: default_value)

ch_type_mapping.update(map_chs)

return ch_type_mapping

I only noticed something must be wrong with my channels when trying to plot topomaps, and MNE complained about missing location info for some of the channels. I would have preferred to get an error message early on if a channel type cannot be mapped correctly from BIDS to MNE.

So what happened in my was that I somehow, silently, ended up with incorrect channel assignments due to the default dict (how exactly this happened I would still need to investigate)

@sappelhoff @adam2392 I would love to hear your view on this too :)

sappelhoff · 2020-07-21T18:58:46Z

mmmh yes, it sounds like defaultdict is a dangerous code practice here. We should handle our cases explicitly and fail/warn early.

sappelhoff · 2020-07-21T19:00:00Z

doc/whats_new.rst

@@ -33,6 +33,8 @@ Changelog
 - :func:`write_raw_bids` now adds citations to the README, by `Alex Rockhill`_ (`#463 <https://github.com/mne-tools/mne-bids/pull/463>`_)
 - :func:`make_dataset_description` now has an additional parameter ``dataset_type`` to set the recommended field ``DatasetType`` (defaults to ``"raw"``), by `Stefan Appelhoff`_ (`#472 <https://github.com/mne-tools/mne-bids/pull/472>`_)
 - :func:`mne_bids.copyfiles.copyfile_brainvision` now has an ``anonymize`` parameter to control anonymization, by `Stefan Appelhoff`_ (`#475 <https://github.com/mne-tools/mne-bids/pull/475>`_)
+- :func:`mne_bids.read_raw_bids` and :func:`mne_bids.write_raw_bids` now map respiratory (``RESP``) channel types, by `Richard Höchenberger` (`#482 <https://github.com/mne-tools/mne-bids/pull/482>`_)
+- :func:`mne_bids.read_raw_bids` now tries to correctly map channel types that are not in all-uppercase letters. Since this is in violation of BIDS, a warning is raised, by `Richard Höchenberger` (`#482 <https://github.com/mne-tools/mne-bids/pull/482>`_)


not sure if I would say this in the changelog ... i'd rather treat it silently. --> but let's see, we are not done discussing anyway, with Mainak's point that BIDS is agnostic at best, ambiguous at worst about casing

Another (temporary?) solution for MNE-BIDS could also be to always convert to uppercase when reading BIDS channel types, i.e.,

for ch_name, ch_type in zip(ch_names_tsv, ch_types_json): updated_ch_type = bids_to_mne_ch_types.get(ch_type.upper(), None) # <-- !!! if updated_ch_type is not None: channel_type_dict[ch_name] = updated_ch_type

@jasmainak WDYT?

@sappelhoff
I've dropped this changelog entry.

I think we need to decide: 1) how specification treats the cases, 2) whether the validator is an accurate reflection of the specification, and 3) what we do in MNE-BIDS about it?

From a user perspective, I think it's nicer if the cases are taken care of in MNE-BIDS without complaining. E.g., through blah.upper() or blah.lower(). You have a similar situation with email addresses: you expect people to use the same casing but actually it doesn't matter. I would still have the validator be more whiny so that we are more forgiving to downstream applications that are not as careful as MNE-BIDS.

agramfort

merge when you're happy here.

I am really happy to see that actually using more and more BIDS dataset to do automatic processing we find all these issues !

mne_bids/read.py

agramfort · 2020-07-22T11:59:35Z

@sappelhoff or @jasmainak feel free to merge

jasmainak · 2020-07-22T17:40:02Z

thanks a ton @hoechenberger !

hoechenberger added 2 commits July 21, 2020 18:03

Make flake8 happy

653c035

jasmainak reviewed Jul 21, 2020

View reviewed changes

mne_bids/tests/test_write.py Outdated Show resolved Hide resolved

Remove debugging cruft

4bc487d

sappelhoff reviewed Jul 21, 2020

View reviewed changes

agramfort reviewed Jul 21, 2020

View reviewed changes

jasmainak reviewed Jul 22, 2020

View reviewed changes

mne_bids/read.py Show resolved Hide resolved

sappelhoff mentioned this pull request Jul 22, 2020

Should _get_ch_type_mapping() ignore case of input data? #481

Closed

hoechenberger added 4 commits July 22, 2020 13:36

Reference issue

e4baa4c

Add test

3818404

Remove changelog entry

fded2b0

Add docstring

81dd026

agramfort approved these changes Jul 22, 2020

View reviewed changes

jasmainak merged commit 023b25d into mne-tools:master Jul 22, 2020

hoechenberger deleted the ch_types branch July 22, 2020 17:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make ch_type reading more robust & support RESP #482

Make ch_type reading more robust & support RESP #482

hoechenberger commented Jul 21, 2020

codecov-commenter commented Jul 21, 2020 •

edited

jasmainak commented Jul 21, 2020

hoechenberger commented Jul 21, 2020 •

edited

sappelhoff commented Jul 21, 2020

sappelhoff Jul 21, 2020

hoechenberger Jul 21, 2020 •

edited

hoechenberger Jul 22, 2020

hoechenberger Jul 22, 2020

jasmainak Jul 22, 2020

agramfort left a comment

agramfort commented Jul 22, 2020

jasmainak commented Jul 22, 2020

Make ch_type reading more robust & support RESP #482

Make ch_type reading more robust & support RESP #482

Conversation

hoechenberger commented Jul 21, 2020

PR Description

Merge checklist

codecov-commenter commented Jul 21, 2020 • edited

Codecov Report

jasmainak commented Jul 21, 2020

hoechenberger commented Jul 21, 2020 • edited

sappelhoff commented Jul 21, 2020

sappelhoff Jul 21, 2020

Choose a reason for hiding this comment

hoechenberger Jul 21, 2020 • edited

Choose a reason for hiding this comment

hoechenberger Jul 22, 2020

Choose a reason for hiding this comment

hoechenberger Jul 22, 2020

Choose a reason for hiding this comment

jasmainak Jul 22, 2020

Choose a reason for hiding this comment

agramfort left a comment

Choose a reason for hiding this comment

agramfort commented Jul 22, 2020

jasmainak commented Jul 22, 2020

codecov-commenter commented Jul 21, 2020 •

edited

hoechenberger commented Jul 21, 2020 •

edited

hoechenberger Jul 21, 2020 •

edited