Skip to content

Add physical_range="channelwise" for EDF export#12510

Merged
mscheltienne merged 7 commits intomne-tools:mainfrom
cbrnr:phys-range-edf-export
Mar 25, 2024
Merged

Add physical_range="channelwise" for EDF export#12510
mscheltienne merged 7 commits intomne-tools:mainfrom
cbrnr:phys-range-edf-export

Conversation

@cbrnr
Copy link
Copy Markdown
Contributor

@cbrnr cbrnr commented Mar 22, 2024

Fixes #12493 by adding physical_range="channelwise". The default behavior remains physical_range="auto", which sets the physical range per channel type. However, in some cases where channels vary wildly in their ranges (mainly due to different offsets), which is often the case if the amplifier used no DC HP filter (e.g. Biosemi), setting individual physical ranges per channel ensures that each channel uses the maximum resolution afforded by the EDF format (only 16 bits). This seems to be perfectly compatible with the EDF standard (please correct me if I'm wrong), but I've added a note that some tools might be incompatible with such EDF files.

@cbrnr cbrnr changed the title Add channelwise option for physical_range Add physical_range="channelwise" for EDF export Mar 22, 2024
@cbrnr cbrnr changed the title Add physical_range="channelwise" for EDF export Add physical_range="per-channel" for EDF export Mar 22, 2024
@cbrnr cbrnr marked this pull request as ready for review March 22, 2024 08:19
@cbrnr cbrnr added this to the 1.7 milestone Mar 22, 2024
Copy link
Copy Markdown
Member

@mscheltienne mscheltienne left a comment

Choose a reason for hiding this comment

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

LGTM, one comment I can not add in-line, in the docdict entry export_edf_note that you modified, could you replace:

:attr:`raw.set_channel_types <mne.io.Raw.set_channel_types>`

with

:meth:`mne.io.Raw.set_channel_types`

Same with set_montage 2 lines later.

Copy link
Copy Markdown
Contributor Author

@cbrnr cbrnr left a comment

Choose a reason for hiding this comment

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

Done @mscheltienne. BTW, LMK if "per-channel" follows MNE conventions, or if "per_channel" would be better (or if you have a better term).

@drammock
Copy link
Copy Markdown
Member

Done @mscheltienne. BTW, LMK if "per-channel" follows MNE conventions, or if "per_channel" would be better (or if you have a better term).

apply_function has a param "channelwise". That would avoid the question of whether to use hyphen or underscore. Off the top of my head IDK if we're consistent about that for multi word special string param values

@mscheltienne
Copy link
Copy Markdown
Member

Maybe let's go with the str channelwise to match that argument name, but I would be against adding a new parameter to the function.
But no strong feelings either way.

@drammock
Copy link
Copy Markdown
Member

Maybe let's go with the str channelwise to match that argument name, but I would be against adding a new parameter to the function.

agreed; sorry my last message wasn't clear but i did mean using the string, not adding a separate param

Copy link
Copy Markdown
Contributor Author

@cbrnr cbrnr left a comment

Choose a reason for hiding this comment

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

Initially, I used channelwise, but then thought that the correct spelling was channel-wise, which doesn't look so nice as an argument. Anyway, I've now switched (back) and this should be good to go!

@cbrnr cbrnr changed the title Add physical_range="per-channel" for EDF export Add physical_range="channelwise" for EDF export Mar 25, 2024
@mscheltienne mscheltienne merged commit 6c4418c into mne-tools:main Mar 25, 2024
@mscheltienne
Copy link
Copy Markdown
Member

Thanks @cbrnr! I will definitely use this in my EDF exports!

@cbrnr cbrnr deleted the phys-range-edf-export branch March 25, 2024 19:49
@cbrnr
Copy link
Copy Markdown
Contributor Author

cbrnr commented Mar 25, 2024

I'd only use this when you really need it though, because it seems like some programs (EEGLAB?) cannot properly handle EDF files with individual channel ranges. So basically, whenever you have zero-mean signals, you don't need it and you can just use "auto".

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.

Resolution issue with EDF export

3 participants