The order of raw1.info["bads"] should not matter when concatenating with raw0.info["bads"] #11501 #11502

moritz-gerster · 2023-02-23T08:41:31Z

Reference issue

What does this implement/fix?

The order of bad channels no longer matters.

Note, that this is already correctly implemented in _ensure_infos_match:

Lines 2903 to 2904 in 603a96d

    
           if set(info1['bads']) != set(info2['bads']): 
        
               raise ValueError(f'{name}.info[\'bads\'] must match')

It's only wrong in _check_raw_compatibility(raw):

mne-python/mne/io/base.py

Lines 2501 to 2513 in 603a96d

    
           def _check_raw_compatibility(raw): 
        
               """Ensure all instances of Raw have compatible parameters.""" 
        
               for ri in range(1, len(raw)): 
        
                   if not isinstance(raw[ri], type(raw[0])): 
        
                       raise ValueError(f'raw[{ri}] type must match') 
        
                   for key in ('nchan', 'bads', 'sfreq'): 
        
                       a, b = raw[ri].info[key], raw[0].info[key] 
        
                       if a != b: 
        
                           raise ValueError( 
        
                               f'raw[{ri}].info[{key}] must match:\n' 
        
                               f'{repr(a)} != {repr(b)}') 
        
                   if not set(raw[ri].info['ch_names']) == set(raw[0].info['ch_names']): 
        
                       raise ValueError('raw[%d][\'info\'][\'ch_names\'] must match' % ri)

mne/io/base.py

Co-authored-by: Eric Larson <larson.eric.d@gmail.com>

moritz-gerster · 2023-02-24T07:37:42Z

Shit I got confused by git. I hope I didn't mess everything up. I think some files were moved to the cloud during a commit, that might be why there are so many file changes. How can I fix this?

This reverts commit bff61e8.

larsoner · 2023-02-24T14:32:53Z

Yikes, a ton of files deleted :)

I looked through your commits and saw which one did this (bff61e8), and then did:

git revert bff61e8c0bb03b9385b7ebad548f66023cb650a9
git checkout bff61e8c0bb03b9385b7ebad548f66023cb650a9 mne/io/base.py
git commit -am 'FIX: Try this'
git push

Looks like the diff is now +24 −13 so probably closer to correct at least. Locally if you git pull origin compare_bads you should get to the same state, then you can fix whatever is still broken/missing!

mne/io/base.py

larsoner · 2023-02-24T14:36:07Z

mne/io/base.py

+        bads_mismatch = bads0.symmetric_difference(badsi)
+        if bads_mismatch:
+            raise ValueError(f'raw[{i}][\'info\'][\'bads\'] do not match: '
+                             f'{sorted(bads_mismatch)}')
+        chs0 = set(raw[0].info['ch_names'])
+        chsi = set(raw[i].info['ch_names'])
+        ch_mismatch = chs0.symmetric_difference(chsi)


This code also could be more DRY -- it should exist in a loop like

for kind in ('bads', 'ch_names'):

... but this also now does a symmetric diff on the channel names, which I don't think we've discussed yet. I actually don't think this will work -- I'd expect the user to need to do a minual reorder_channels before being able to concat successfully/correctly. So for ch_names the order does actually matter...

@moritz-gerster I think this comment has not been addressed. I do not think you should use set with channels like you do (now) for bads, so I think this chs part should be reverted / go back to old behavior, ideally with a test that fails if the channel orders do not match (it should IIRC!)

@moritz-gerster I think this comment has not been addressed. I'll un-resolve it.

I noticed on this read through that it's a set check on main so you can ignore that part -- but I think it could still be more DRY by using a loop, right?

moritz-gerster · 2023-02-24T19:57:14Z

Looks like the diff is now +24 −13 so probably closer to correct at least. Locally if you git pull origin compare_bads you should get to the same state, then you can fix whatever is still broken/missing!

Thank you for your help! ☺️

moritz-gerster · 2023-03-02T08:03:26Z

Dear @larsoner, are the changes good to go now? :)

agramfort

diff looks good @moritz-gerster

any chance you can update or add a test for this?

moritz-gerster · 2023-03-03T11:54:01Z

any chance you can update or add a test for this?

@agramfort: Check ✅

agramfort

@moritz-gerster there is a already a place where concatenate_raws is tested. https://github.com/mne-tools/mne-python/blob/main/mne/io/fiff/tests/test_raw_fiff.py#L366

I know it's not the ideal place and it's there cause back in the days when this was added we were only supporting FIF files. Can you move your testing there to be consistent? Also rename your test test_concatenate_raws_bads_order

thanks

moritz-gerster · 2023-03-03T14:52:15Z

Can you move your testing there to be consistent? Also rename your test test_concatenate_raws_bads_order

@agramfort done :)

larsoner

Marking as requesting changes so we don't forget to undo the chs-as-set check

moritz-gerster · 2023-03-15T19:04:20Z

@larsoner is there more I should do? :)

agramfort · 2023-03-15T20:59:24Z

@moritz-gerster you need to fix conflicts

moritz-gerster · 2023-03-16T08:02:35Z

@moritz-gerster you need to fix conflicts

@agramfort done ✅

agramfort

@larsoner I let you merge

moritz-gerster · 2023-03-19T07:56:21Z

@larsoner good to go now?

Some tests are failing but they don't seem to be related to my changes. They only failed after I synchronized the code with the main branch

agramfort · 2023-03-19T10:24:00Z

@moritz-gerster lets wait for main to be green and lets merge this after its green again

larsoner · 2023-03-19T17:22:13Z

Thanks @moritz-gerster !

* upstream/main: Fix bug in new `mne.sys_info()` (mne-tools#11577) Remove legacy plot_psd* funcs/methods in our docs and codebase (mne-tools#11563) The order of raw1.info["bads"] should not matter when concatenating with raw0.info["bads"] mne-tools#11501 (mne-tools#11502) MAINT: Fixes for matplotlib and pandas (mne-tools#11574) Fix Circle [circle deploy] Fix export to EDF format with a set physical range smaller than the data range (mne-tools#11569) Slightly rework `mne.sys_info()` (mne-tools#11568)

moritz-gerster added 2 commits February 23, 2023 09:38

compare bads set instead of bads lists

2be2589

Merge branch 'main' into compare_bads

a10654d

larsoner reviewed Feb 23, 2023

View reviewed changes

mne/io/base.py Outdated Show resolved Hide resolved

larsoner reviewed Feb 23, 2023

View reviewed changes

mne/io/base.py Outdated Show resolved Hide resolved

moritz-gerster and others added 2 commits February 24, 2023 07:47

Style correction

ea7b45f

Co-authored-by: Eric Larson <larson.eric.d@gmail.com>

more informative error message

bff61e8

moritz-gerster requested a review from sappelhoff as a code owner February 24, 2023 07:13

Merge branch 'main' into compare_bads

54a150d

moritz-gerster and others added 3 commits February 24, 2023 08:42

update doc

1fdbec5

Revert "more informative error message"

d669a1c

This reverts commit bff61e8.

FIX: Try this

5355d8e

larsoner reviewed Feb 24, 2023

View reviewed changes

mne/io/base.py Outdated Show resolved Hide resolved

larsoner reviewed Feb 24, 2023

View reviewed changes

ri as index

cc520b4

agramfort reviewed Mar 2, 2023

View reviewed changes

add test

3340008

agramfort reviewed Mar 3, 2023

View reviewed changes

move test file

f74909d

larsoner requested changes Mar 3, 2023

View reviewed changes

agramfort approved these changes Mar 3, 2023

View reviewed changes

moritz-gerster added 2 commits March 16, 2023 07:59

remove change entry

775fe84

Merge branch 'main' into compare_bads

bfc93c5

Add doc

8cccffa

moritz-gerster requested review from larsoner and removed request for sappelhoff March 16, 2023 08:02

agramfort approved these changes Mar 16, 2023

View reviewed changes

moritz-gerster added 5 commits March 16, 2023 17:29

more DRY

c4e5b30

PEP8

8acc3b6

resolve conflcits

25e9c54

Merge branch 'main' into compare_bads

ebf74e1

Update doc

167629b

larsoner mentioned this pull request Mar 19, 2023

MAINT: Fixes for matplotlib and pandas #11574

Merged

Merge branch 'main' into compare_bads

b26f951

larsoner approved these changes Mar 19, 2023

View reviewed changes

larsoner merged commit 0e305ee into mne-tools:main Mar 19, 2023

	if set(info1['bads']) != set(info2['bads']):
	raise ValueError(f'{name}.info[\'bads\'] must match')

	def _check_raw_compatibility(raw):
	"""Ensure all instances of Raw have compatible parameters."""
	for ri in range(1, len(raw)):
	if not isinstance(raw[ri], type(raw[0])):
	raise ValueError(f'raw[{ri}] type must match')
	for key in ('nchan', 'bads', 'sfreq'):
	a, b = raw[ri].info[key], raw[0].info[key]
	if a != b:
	raise ValueError(
	f'raw[{ri}].info[{key}] must match:\n'
	f'{repr(a)} != {repr(b)}')
	if not set(raw[ri].info['ch_names']) == set(raw[0].info['ch_names']):
	raise ValueError('raw[%d][\'info\'][\'ch_names\'] must match' % ri)

Uh oh!

The order of raw1.info["bads"] should not matter when concatenating with raw0.info["bads"] #11501 #11502

The order of raw1.info["bads"] should not matter when concatenating with raw0.info["bads"] #11501 #11502

Uh oh!

Conversation

moritz-gerster commented Feb 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reference issue

What does this implement/fix?

Uh oh!

Uh oh!

Uh oh!

moritz-gerster commented Feb 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

larsoner commented Feb 24, 2023

Uh oh!

Uh oh!

larsoner Feb 24, 2023

Choose a reason for hiding this comment

Uh oh!

larsoner Mar 3, 2023

Choose a reason for hiding this comment

Uh oh!

larsoner Mar 16, 2023

Choose a reason for hiding this comment

Uh oh!

moritz-gerster commented Feb 24, 2023

Uh oh!

moritz-gerster commented Mar 2, 2023

Uh oh!

agramfort left a comment

Choose a reason for hiding this comment

Uh oh!

moritz-gerster commented Mar 3, 2023

Uh oh!

agramfort left a comment

Choose a reason for hiding this comment

Uh oh!

moritz-gerster commented Mar 3, 2023

Uh oh!

larsoner left a comment

Choose a reason for hiding this comment

Uh oh!

moritz-gerster commented Mar 15, 2023

Uh oh!

agramfort commented Mar 15, 2023

Uh oh!

moritz-gerster commented Mar 16, 2023

Uh oh!

agramfort left a comment

Choose a reason for hiding this comment

Uh oh!

moritz-gerster commented Mar 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

agramfort commented Mar 19, 2023

Uh oh!

larsoner commented Mar 19, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

moritz-gerster commented Feb 23, 2023 •

edited

Loading

moritz-gerster commented Feb 24, 2023 •

edited

Loading

moritz-gerster commented Mar 19, 2023 •

edited

Loading