raw.add_channels 'buffer_size_sec' error

[kingjr]

I'm trying to add a stim channel (a clean combination of multiple already existing stim channels) to a raw object:

# read raw
raw = read_raw_edf(fname, preload=True)

# clean stim
raw_stim = raw.pick_channels(chs_stim, copy=True)
data = clean_stim(raw_stim._data)

# append data
info = create_info(ch_names=['stim_clean'], sfreq=raw.info['sfreq'], ch_types=['stim'])
raw_stim = RawArray(data, info=info, first_samp=raw._first_samps[0])
raw.add_channels([raw_stim])

But I get the following error:

RuntimeError: Don't know how to merge 'buffer_size_sec'. Make sure values are compatible.

What shall I do?

cc @choldgraf @Eric89GXL

[larsoner]

Set buffer_size_sec in create_info to whatever it is in raw, i.e.:

create_info(..., buffer_size_sec=raw.info['buffer_size_sec'])

[kingjr]

TypeError: create_info() got an unexpected keyword argument 'buffer_size_sec'

[larsoner]

info = create_info(...)
info['buffer_size_sec'] = raw.info['buffer_size_sec']

[kingjr]

However I can do: info['buffer_size_sec'] = raw.info['buffer_size_sec']

But that doesn't feel very clean

[larsoner]

It's a fine way to do it. We don't expose all options in create_info but you can set them afterward. We could change this behavior.

[kingjr]

And now I have: RuntimeError: Don't know how to merge 'filename'. Make sure values are compatible.

when I add the channel raw.add_channels([raw_stim])

[kingjr]

Overall, it seems like an aweful lot of steps to just add a stim channel..

[kingjr]

It works with

        info['buffer_size_sec'] = raw.info['buffer_size_sec']
        info['filename'] = raw.info['filename']

I'm keeping the issue opened. I think we should have a less hacky way. WDYT?

I would expect to be able to do Raw.add_channels(data=np.array, ch_type=['stim', 'misc'], ch_names=['foo', 'bar'])

[larsoner]

for key in ('buffer_size_sec', 'filename'):
    ...

is how I would do it to make it DRY.

I know it is more work, but it is also more explicit. It forces the user to think about what keys are being merged and what the values should actually be, which isn't necessarily a bad thing. I'm open to ideas about how to make it more automated. However, with automation there is the risk of getting burned by not understanding what's going on under the hood. As I mentioned in another issue, info actually stores a lot of information, and thinking of it as just containing channel types and names can cause problems.

If adding a stim channel to data that does not have one is a common enough use case, maybe we should add a function specific for that. Stim channels in general have less information than, say, EEG or MEG. Then the user won't need to care about info at all.

[kingjr]

Stim channels in general have less information than, say, EEG or MEG. Then the user won't need to care about info at all.

+1

[choldgraf]

If you don't have a problem totally obliterating what's in your info channel (e.g., if you're doing this with ecog or something that doesn't have montage metadata etc), then what I've done is first re-create my dataset using RawArray, and then append the stim channel to it. Agreed that it removes a bunch of information.

That said, I think it'd be helpful to have documentation about what fields will result in an error if you try to merge a manually-created info object with one read from an EDF file or something.

[choldgraf]

What if there were a function to merge info objects that behaved similarly to how pandas merges dataframes? E.g., it could have flags to either throw an error with a field mismatch, or to replace values using one or the other info object. Then any time that a field was overwritten it could write that to the log.

[kingjr]

I think it's better to not create a RawArray from scratch, just in case we forget some info.

This is what I currently do

def add_channels(inst, data, ch_names, ch_types):
    from mne.io import _BaseRaw, RawArray
    from mne.epochs import _BaseEpochs, EpochsArray
    from mne import create_info
    if 'meg' in ch_types or 'eeg' in ch_types:
        return NotImplementedError('Can only add misc, stim and ieeg channels')
    info = create_info(ch_names=ch_names, sfreq=inst.info['sfreq'],
                       ch_types=ch_types)
    if isinstance(inst, _BaseRaw):
        for key in ('buffer_size_sec', 'filename'):
            info[key] = inst.info[key]
        new_inst = RawArray(data, info=info, first_samp=inst._first_samps[0])
    elif isinstance(inst, _BaseEpochs):
        new_inst = EpochsArray(data, info=info)
    else:
        raise ValueError('unknown inst type')
    return inst.add_channels([new_inst], copy=True)

[agramfort]

how about:

read raw

raw = read_raw_edf(fname, preload=True)

clean stim

raw_stim = raw.pick_channels(chs_stim, copy=True)raw_stim._data = clean_stim(raw_stim._data)

append data

raw.add_channels([raw_stim])

?

[kingjr]

The raw_stim has multiple chans (one for each binary TTL) whereas the clean_stim is the combination (similar to STI 101 in elekta defaults).

One could however follow your solution and add an additional pick_channels, and perhaps a rename_channels afterward.

However, if one (i.e. me) wants to add, say, an auditory misc channel, then you need to change the ch_type (or already have a misc in your raw).

Overall, there's more than one hack that'll do the trick, I was mainly asking what should be the cleanest way.

(I don't want to reopen a fight on small API changes; I'm personally just going to use the code above, that seems clean and generic enough to me)

[agramfort]

I would be fine adding a "force" param to raw.add_channels to drop any offending key in the Info and be therefore more permissive

[choldgraf]

That sounds good to me - it'd just force any overlapping keys w/ different values to take on the value of the self object, and raise a warning? Can anybody think of an instance where this might break things?

[larsoner]

That's okay with me as long as it's opt-in by kwarg, not default

[agramfort]

PR welcome then :)

[choldgraf]

can do this, but not right now...it will go on my list of "fun things to do once I resubmit my paper" :)

On Mon, Feb 29, 2016 at 1:41 PM, Alexandre Gramfort < notifications@github.com> wrote:

PR welcome then :)

— Reply to this email directly or view it on GitHub https://github.com/mne-tools/mne-python/issues/2948#issuecomment-190408411 .

[kingjr]

I would be fine adding a "force" param to raw.add_channels to drop any offending key in the Info and be therefore more permissive

+1

can do this, but not right now...it will go on my list of "fun things to do once I resubmit my paper" :)

Ok, good luck ;)

[larsoner]

@choldgraf certainly your paper is resubmitted by now? :)

[choldgraf]

:(

[choldgraf]

let me put it this way: I'm not shaving or getting a haircut until I resubmit the damn paper, and yesterday my labmate said that I had a nice "The Revenant" thing going on...

[larsoner]

I'm going to move this to 0.13 but if you get to it by mid/end of April then it's fine to put in 0.12

[choldgraf]

OK - the tentative plan is to resubmit by the end of march, so that does seem reasonable. This thread is on my to-do list so I won't forget about it

[choldgraf]

Looking through this now. I'm not quite sure the best way to add a "force" option. The reason is that there's a lot of machinery under the hood to make sure that values are able to be merged between info objects. So here are a couple of thoughts:

1. Hard-code some fields that we know are going to be problematic for adding stim channels, and if a "force" parameter is given, then automatically add those fields to new info objects before merging (similar to what @kingjr and @Eric89GXL do above)
2. Separate from the merging code, write some kind of make_info_mergeable function that takes a base info object and a list of other info objects, and does something like 1 above for the to-be-appended info objects.
3. Write a more general function that has a force parameter which will overwrite all inconsistencies between the base / to-append info with those of the base info object.

If we think that the problematic fields won't change much in the future, I think that 1 might be the most verbose and easy to implement, but it might be a pain if we think that the fields which throw this error will change over time.

WDYT?

[larsoner]

Would this work?

1. The force option takes any conflicting keys and uses the value from the first dictionary (easy to explain, hopefully easy to implement). Call _check_consistency at the end, and beef up that function of necessary to hopefully catch any traps.