Allow exporting STIM channels to EDF #12332
Conversation
cbrnr
requested review from
larsoner,
drammock,
agramfort and
dengemann
as code owners
|
@hofaflo currently the unit for exporting is hard-coded to |
| # Add warning if any channel types are not voltage based. | ||
| # Users are expected to only export data that is voltage based, | ||
| # such as EEG, ECoG, sEEG, etc. | ||
| # Non-voltage channels are dropped by the export function. | ||
| # Note: we can write these other channels, such as 'misc' | ||
| # but these are simply a "catch all" for unknown or undesired | ||
| # channels. | ||
| voltage_types = list(units) + ["stim", "misc"] | ||
| non_voltage_ch = [ch not in voltage_types for ch in orig_ch_types] | ||
| if any(non_voltage_ch): | ||
| warn( | ||
| f"Non-voltage channels detected: {non_voltage_ch}. MNE-Python's " | ||
| "EDF exporter only supports voltage-based channels, because the " | ||
| "EDF format cannot accommodate much of the accompanying data " | ||
| "necessary for channel types like MEG and fNIRS (channel " | ||
| "orientations, coordinate frame transforms, etc). You can " | ||
| "override this restriction by setting those channel types to " | ||
| '"misc" but no guarantees are made of the fidelity of that ' | ||
| "approach." | ||
| ) |
There was a problem hiding this comment.
IIUC, the goal is to write STIM channels. But I don't get why that entails removing this warning? As the edited test shows, now there is no warning when writing e.g. hbr channels. Can you explain the logic of why this is being removed?
There was a problem hiding this comment.
Because that warning doesn't make sense to me. I know it's not specific to STIM channels, but EDF can accommodate arbitrary signal types. Just because some domain-specific meta-information cannot be stored is not really an argument against non-voltage channels (especially since we don't store e.g. channel locations for EEG data either). Also, converting to "misc" was not necessary as the warning suggests, so it is misleading.
There was a problem hiding this comment.
we don't store e.g. channel locations for EEG data either
oh wow, OK. I'd forgotten that. I just looked at the docstring of export_raw() and it does already have appropriate warnings in the Notes section (though they do say "stim channels will be dropped" --- you should change that in this PR).
Co-authored-by: Daniel McCloy <[email protected]>
|
Looks like you've addressed the remaining comments and it's green, thanks @cbrnr ! |
Co-authored-by: Daniel McCloy <[email protected]>
Fixes #9883.