[MRG, BUG] Fixed bug with EDF channel types and minor bug where other types of channels were not written and bug where annotations were not written to correct scale

[adam2392]

Reference issue

Follow up fix from #9643 where not all the relevant "Voltage/uVoltage" channels were added.

In addition, it will allow channel types to be written to EDF files, in accordance with: #9694 (comment)

Closes: #9704
Closes: #9730

What does this implement/fix?

• Adds corresponding unit test that makes sure all voltage channels are covered in exporting EDF
• Allows channel types to be read in from EDF files via their signal labels
• Allows exporting channel types to EDF files

[cbrnr]

The test doesn't cover the bio and dbs types you added here. Do we really want these types? The docstring says EDF export only supports EEG, ECoG, and sEEG, so this should be updated in any case (whether or not we add bio and dbs, because we already support eog, ecg, and emg).

[adam2392]

The test doesn't cover the bio and dbs types you added here. Do we really want these types? The docstring says EDF export only supports EEG, ECoG, and sEEG, so this should be updated in any case (whether or not we add bio and dbs, because we already support eog, ecg, and emg).

Oh apologies, forgot to push up the updated test.

Upon further inspection, I realized that EDF cannot write the type of channels in the header, as there is nowhere to store that. I also realized that read_raw_edf will end up losing the channel type information. With that being said, one should be able to write any type of "voltage" type signals into the EDF file, as I have seen EOG, ECG and even EMG before. DBS and Bio are just future-proofing since it doesn't hurt to add them.

What we might need to do though is "specify" that you lose channel type information in the doc string.

However, with packages like mne-bids, this issue is circumvented since one would just leverage the channels.tsv file in BIDS to set channel types.

[agramfort]

no objection to merge

[adam2392]

Okay now I think this is good to go from me. I put the hidden runtime bug that's user dependent into a new issue.

[Teuniz]

Upon further inspection, I realized that EDF cannot write the type of channels in the header, as there is nowhere to store that.

There is:

The header field 'label' offers 16 ASCII characters. The standard structure consists of three components, from left to right:

Type of signal (for example EEG).  
A space.  
Specification of the sensor (for example Fpz-Cz).  

https://edfplus.info/specs/edftexts.html#label

https://edfplus.info/specs/edftexts.html#signals

[adam2392]

Sorry @agramfort there were some updates pertaining to how EDF should be read/written.

This is good to go again on my end, and I have added in all the thoughts from #9704

[agramfort]

what does eeglab do when reading files with ch types like this?

[cbrnr]

what does eeglab do when reading files with ch types like this?

I assume that readers that do not parse the label field will just display the complete text as a channel label, e.g. "EEG Fz" instead of just "Fz". For this reason, I would like to have an option to turn channel type parsing off.

[adam2392]

mne/tests/test_cov.py::test_compute_whitener seems to fail, but I don't think I made any changes affecting that.

[cbrnr]

I'm sorry if you have already mentioned it in this long thread, but how does EDF only support voltage data? The specs just mention that the physical dimension for each channel are 8 ASCII characters, so any unit should be supported.

[adam2392]

I'm sorry if you have already mentioned it in this long thread, but how does EDF only support voltage data? The specs just mention that the physical dimension for each channel are 8 ASCII characters, so any unit should be supported.

Idk actually. I think possibly we could export any channel type, but I would prefer not implementing the ability to export MEG -> EDF. My impression was that EDF should only be used for EEG-like data. I don't see why anyone would export MEG/non-voltage data to EDF and not FIF unless it was by accident, so we have some catches in export to handle that.

Ref: https://www.edfplus.info/specs/edftexts.html

[cbrnr]

I disagree. The table on the specs page even lists MEG as a signal type. I think we should not artificially restrict our export functionality by allowing only voltage data, which is not backed up by the EDF specs.

[cbrnr]

Re exporting to EDF vs. FIFF, I think EDF is much more widely adopted/used than FIFF, but even if FIFF is the de facto standard for MEG signals the EDF format allows signals of any unit to be stored.

[adam2392]

@cbrnr can you lmk which data channels you would like included here? I'm not a MEG user, so I've only ever used MNE for EEG/iEEG.

Would it be:

mag='fT', grad='fT/cm'

Anything else?

[cbrnr]

I would include everything. Why not?

[adam2392]

Actually the challenge is more-so on reading in MEG/fnirs data correctly using read_raw_edf. I'm not sure how to read in this data correctly. Sure I can easily append "MEG M1" to indicate M1 is a MEG channel, or "FNIRS M2" to indicate M2 is a FNIRS channel, but reading the data back in to do a unit test on the round trip is non-trivial for me.

Is it possible to leave this PR as is and then table it for a future improvement from someone who wants to export MEG/Fnirs to EDF and is familiar with the reader of MEG/Fnirs enough to add that functionality in?

Reference LOC:

mne-python/mne/io/edf/edf.py

Lines 388 to 417 in b579d32

	for idx, ch_name in enumerate(ch_names):
	chan_info = {}
	chan_info['cal'] = 1.
	chan_info['logno'] = idx + 1
	chan_info['scanno'] = idx + 1
	chan_info['range'] = 1.
	chan_info['unit_mul'] = FIFF.FIFF_UNITM_NONE
	chan_info['ch_name'] = ch_name
	chan_info['unit'] = FIFF.FIFF_UNIT_V
	chan_info['coord_frame'] = FIFF.FIFFV_COORD_HEAD
	chan_info['coil_type'] = FIFF.FIFFV_COIL_EEG
	chan_info['kind'] = FIFF.FIFFV_EEG_CH
	chan_info['loc'] = np.zeros(12)
	if ch_name in eog or idx in eog or idx - nchan in eog:
	chan_info['coil_type'] = FIFF.FIFFV_COIL_NONE
	chan_info['kind'] = FIFF.FIFFV_EOG_CH
	pick_mask[idx] = False
	elif ch_name in misc or idx in misc or idx - nchan in misc:
	chan_info['coil_type'] = FIFF.FIFFV_COIL_NONE
	chan_info['kind'] = FIFF.FIFFV_MISC_CH
	pick_mask[idx] = False
	elif idx in stim_channel_idxs:
	chan_info['coil_type'] = FIFF.FIFFV_COIL_NONE
	chan_info['unit'] = FIFF.FIFF_UNIT_NONE
	chan_info['kind'] = FIFF.FIFFV_STIM_CH
	pick_mask[idx] = False
	chan_info['ch_name'] = ch_name
	ch_names[idx] = chan_info['ch_name']
	edf_info['units'][idx] = 1
	chs.append(chan_info)

[larsoner]

+1 for doing it later. I assume YAGNI, and for MEG important information will be lost anyway (at the very least, the head<->device transform, sensor orientations, sensor coil types)

[cbrnr]

The same could be said for EEG as well because EDF doesn't store custom sensor locations. But let's stick to exporting EEG for now, the warning message and code comment are now clear that this is not a restriction imposed by EDF.

[adam2392]

I disagree. The table on the specs page even lists MEG as a signal type. I think we should not artificially restrict our export functionality by allowing only voltage data, which is not backed up by the EDF specs.

Okay sure I can add in then MEG support. Can you lmk which channels and their corresponding default units I should add in to this comment? I'm not too familiar with non-EEG data types in MNE.

#9694 (comment)

[adam2392]

https://34137-1301584-gh.circle-artifacts.com/0/dev/generated/mne.export.export_raw.html#mne.export.export_raw

This lgtm to go if we are okay w/ the future PR.

[adam2392]

Don't want to lose track of this PR since it had a lot of discussion.

Is this good to merge now?

[agramfort]

thx @adam2392

[larsoner]

This needed to be nan_arr.copy() (as found with @drammock working together), after this PR all info['chs'][ii]['loc'] point to the same array in memory, so setting values in one of them sets all of them. We need a unit test that would have caught this...

[larsoner]

(I think @drammock is working on a fix for this)

[adam2392]

Ah my bad. I changed to nan to better match roundtrip.

Lmk if you need me to PR

[drammock]

see #9743