Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MRG+1: BUG: info['bads'] order shouldn't matter in write_evokeds() #7954

Merged
merged 3 commits into from
Jul 2, 2020

Conversation

hoechenberger
Copy link
Member

@hoechenberger hoechenberger commented Jul 1, 2020

What does this implement/fix?

I ran into an issue where I created two Evokeds and their difference (via combine_evoked()); say, e1, e2, and diff.

I then wanted to write those Evokeds to a single file via mne.write_evokeds([e1, e2, diff]), which failed with a ValueError: "info['bads'] must match'". I looked into the bads lists and found that the order of bad channels in diff was different from the order in e1 and e2; yet, the set of channels was identical.

While it probably would make sense to figure out where this difference comes from, it also got me thinking that an altered bads order shouldn't be a stopper when using write_evokeds(), as long as all passed Evoked objects mark the same channels as bad.

And this is exactly what this PR implements.

I ran into an issue where I created two Evokeds and their difference
(via combine_evoked); say, `e1`, `e2`, and `diff`.

I then wanted to write those Evokeds to a single file via
`mne.write_evokeds([e1, e2, diff])`, which failed with a
`ValueError`: "info['bads'] must match'". I looked into the `bads`
lists and found that the order of bad channels in `diff` was different
from the order in `e1` and `e2`; yet, the set of channels was identical.

While it probably would make sense to figure out where this difference
comes from, it also got me thinking that an altered `bads` order
shouldn't be a stopper when using `write_evokeds()`, as long as all
passed Evoked objects mark the same channels as bad.

And this is exactly what this commit implements.
Copy link
Member

@larsoner larsoner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM +1 for merge, maybe needs latest.inc update?

@hoechenberger
Copy link
Member Author

@larsoner I've added a changelog entry.

@larsoner larsoner changed the title info['bads'] order shouldn't matter in consistency check performed by write_evokeds() MRG+1: BUG: info['bads'] order shouldn't matter in write_evokeds() Jul 1, 2020
@agramfort agramfort merged commit 517c496 into mne-tools:master Jul 2, 2020
@agramfort
Copy link
Member

Thx @hoechenberger

@hoechenberger hoechenberger deleted the epochs-info-bads branch July 2, 2020 06:43
larsoner added a commit to larsoner/mne-python that referenced this pull request Jul 8, 2020
* upstream/master: (30 commits)
  MRG: Add remove_labels to _Brain (mne-tools#7964)
  Add get_picked_points (mne-tools#7963)
  ENH: Add OpenGL info to mne sys_info (mne-tools#7976)
  [MRG] Fix reject_tmin and reject_tmax for reject_by_annotation in mne.Epochs (mne-tools#7967)
  mrg: Add scalar mult and div operators for AverageTFR (mne-tools#7957)
  MRG, MAINT: Cleaner workaround for Sphinx linking issue (mne-tools#7970)
  MRG, ENH: Speed up epochs.copy (mne-tools#7968)
  MRG, BUG: Allow ref mags to have a comp grade (mne-tools#7965)
  do not forget to pass adjacency (mne-tools#7961)
  [MRG] fix Issue with stc.project after restricting to a label (mne-tools#7950)
  Only process nirx event file if present (mne-tools#7951)
  MRG+1: BUG: info['bads'] order shouldn't matter in write_evokeds() (mne-tools#7954)
  Fix some small glitches introduced via mne-tools#7845 (mne-tools#7952)
  Add time player (mne-tools#7940)
  MAINT: Clean up VTK9 offset array [circle front] (mne-tools#7953)
  MAINT: Skip a few more on macOS (mne-tools#7948)
  fix links [skip travis] (mne-tools#7949)
  MRG, MAINT: Tweak CIs (mne-tools#7943)
  MRG, BUG: Fix vector scaling (mne-tools#7934)
  MRG, VIZ, BUG: handle CSD channel type when topo plotting (mne-tools#7935)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants