Oh wow only after actually looking at the code I realize I totally misunderstood what this method is even supposed to do, based on what I expected from the method name and its docstring.

The docstring says,

Append new channels to the instance.

So what I thought I could do is, just pass an array of data for additional channels (and channel names), and they will be appended to the existing instance.

But actually what this method is supposed to do is in fact a concatenation of multiple objects of the same type, right?

But we already have .append() and concatenate_*() for this. The only issue with these is that they currently only work on the time domain. But if we added a switch to allow for concatenation on the channel domain as an alternative way of operation, we wouldn't even need .add_channels() anymore.

I guess I'm thinking sort of like in terms of pd.concat(), which has an axis parameter.

Any thoughts here?

it needs to be more than channel name and array as a channel info["chs"] is a lot more than this.

Exactly! That's why naming the method add_channels() is misleading – effectively, you're concatenating multiple "complete" objects.

Look just how much code I had to produce to add two channels to an existing raw:

# Add GSR and temperature channels
gsr_data = np.array([2.1e-6] * len(raw.times))
temperature_data = np.array([36.5] * len(raw.times))

gsr_and_temp_data = np.concatenate([
    np.atleast_2d(gsr_data),
    np.atleast_2d(temperature_data),
])
gsr_and_temp_info = mne.create_info(
    ch_names=["GSR", "Temperature"],
    sfreq=raw.info["sfreq"],
    ch_types=["gsr", "temperature"],
)
gsr_and_temp_info["line_freq"] = raw.info["line_freq"]
gsr_and_temp_info["subject_info"] = raw.info["subject_info"]
with gsr_and_temp_info._unlock():
    gsr_and_temp_info["lowpass"] = raw.info["lowpass"]
    gsr_and_temp_info["highpass"] = raw.info["highpass"]
gsr_and_temp_raw = mne.io.RawArray(
    data=gsr_and_temp_data,
    info=gsr_and_temp_info,
    first_samp=raw.first_samp,
)
raw.add_channels([gsr_and_temp_raw])
del gsr_and

https://github.com/mne-tools/mne-bids/blob/036dc7e08c5053e218f4aeff7c1786ffbdf0a6b4/mne_bids/tests/data/tiny_bids/code/make_tiny_bids_dataset.py#L46-L69

+1 to improve the docs based on the 4 points in the OP

once that is clarified, I think it's easier to live with the fact that add_channels actually concatenates raws (or other instances).

I am not sure what you suggest here.

not sure either, but perhaps that add_channels would be a function that takes params like data, ch_name, ... (and all else that is needed) instead of a raw/... object?

Look just how much code I had to produce to add two channels to an existing raw:

Agree that this is way too difficult. Comments below:

# Add GSR and temperature channels
gsr_data = np.array([2.1e-6] * len(raw.times))
temperature_data = np.array([36.5] * len(raw.times))
gsr_and_temp_data = np.concatenate([
    np.atleast_2d(gsr_data),
    np.atleast_2d(temperature_data),
])

The above could be simplified a bit; still 3 expressions but much shorter:

gsr = np.full((1, len(raw.times)), 2.1e-6)
temp = np.full((1, len(raw.times)), 36.5)
gsr_and_temp_data = np.vstack([gsr, temp])

This next bit seems unaviodable but not so bad:

gsr_and_temp_info = mne.create_info(
    ch_names=["GSR", "Temperature"],
    sfreq=raw.info["sfreq"],
    ch_types=["gsr", "temperature"],
)

Are the below lines necessary when creating RawArray, or only when adding the channels to the existing Raw? (I suspect the latter, but haven't checked). Regardless, we should make it possible to skip line_freq, lowpass, highpass, and subject_info for cases like this --- perhaps by having "wildcard" defaults that will automatically take on the values of the Raw they are concatenated with? This is (I think) what force_update_info does already: uses the existing Raw's info to update/override the info of the concatenated objects. But it would be nice if that were possible without having to first explicitly set values for those info fields.

gsr_and_temp_info["line_freq"] = raw.info["line_freq"]
gsr_and_temp_info["subject_info"] = raw.info["subject_info"]
with gsr_and_temp_info._unlock():
    gsr_and_temp_info["lowpass"] = raw.info["lowpass"]
    gsr_and_temp_info["highpass"] = raw.info["highpass"]

The rest seems unavoidable, but if the two simplifications above were made, I think we're down to something reasonable/acceptable.

gsr_and_temp_raw = mne.io.RawArray(
    data=gsr_and_temp_data,
    info=gsr_and_temp_info,
    first_samp=raw.first_samp,
)
raw.add_channels([gsr_and_temp_raw])

As for the intuitiveness of the API name... I think I would agree that something like raw.add_channels(data, ch_name, ch_type, ...) ought to work too. I can imagine an API where data can be a NumPy array (in which case you must pass other args), or alternatively data can be a list of Raw instances (or epochs or evoked).

I am not sure what you suggest here.

not sure either, but perhaps that add_channels would be a function that takes params like data, ch_name, ... (and all else that is needed) instead of a raw/... object?

I am not sure myself either 😆

But I suppose my feeling was that:

• add_channels() doesn't live up to its name, because
• it performs a concatenation of two same-typed objects, hence
• this functionality could (should) live in concatenate_*(), meaning that
• concatenate_*() should gain an axis-like parameter, and
• add_channels() could be subsequently deprecated or "legacy-lized" (and intenally call concatenate_*()),
• but it would still be super nice if I could just pass an array of data, channel names, channel types, and perhaps locations to add_channels() or a similar method to simply append those channels without having to create an entirely new Raw/Epochs/Evoked container first; but I do realize that things can get out of hand here quickly

Thanks @drammock for the simplifications, that's helpful!

As for the intuitiveness of the API name... I think I would agree that something like raw.add_channels(data, ch_name, ch_type, ...) ought to work too.

I would rather not add this. To me it's replicating RawArray without much gain. I don't think this extra line is worth the API duplication.

Can we just live with a documentation improvement for now?

Can we just live with a documentation improvement for now?

Yep, let's do that!

Hey! who said I'd be volunteering? 😅😅😅😅

I believe @agramfort calls this being "volun-told" :)

More seriously if you won't have time in the next few days @drammock or I could give it a shot. But I think you're the best equipped having most recently suffered through the doc badness :)

Milestone on this has been bumped six times, removing milestone until someone champions this issue. But also marking as "easy" so it hopefully gets visibility / higher likelihood of being tackled

As for the intuitiveness of the API name... I think I would agree that something like raw.add_channels(data, ch_name, ch_type, ...) ought to work too.

I would rather not add this. To me it's replicating RawArray without much gain. I don't think this extra line is worth the API duplication.

I have added some documentation in #13051, however, having this as a convenience function without having to worry about potentially overwriting things in the info would be nice. I tried adding a simple RawArray with a minimal info and there was an exception thrown about inconsistencies in the infos, which I only solved via force_update_info=True, which was odd as the info was mainly empty.

Having a simple function add_channels(data, ch_name) would be more what I expected from the function