Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Error in Physical maximum and minimum values for EDF in writeeeg #246

Closed
SparshJ opened this issue Jan 17, 2021 · 23 comments · Fixed by #381
Closed

Error in Physical maximum and minimum values for EDF in writeeeg #246

SparshJ opened this issue Jan 17, 2021 · 23 comments · Fixed by #381

Comments

@SparshJ
Copy link

SparshJ commented Jan 17, 2021

Hey!

While using the writeeeg function on a data matrix ( channels X samples), the resultant EDF shows different physical maximum and minimum values for different channels even though no such info is passed to writeeeg. This error occurs only for some subjects, whereas for other subjects the physical maximum and minimum values are identical across the channels. Please help!

Thanks!!

@arnodelorme
Copy link
Collaborator

arnodelorme commented Feb 1, 2021

Would you mind providing an example demonstrating the problem? We will look into it.

@SparshJ
Copy link
Author

SparshJ commented Feb 1, 2021

Sure!
I've attached the screenshot below:
EEGLab
As you can see, the physical max and min values are not consistent across the channels. As a result, its not possible to create bipolar montages by subtracting two channels. I think that the swrite function called by pop_biosig's writeeeg calculates these physical max and min values, and that's where this discrepancy is occurring.

Thanks!

@arnodelorme
Copy link
Collaborator

Would you mind sharing the file you are trying to export. It is unclear what the spreadsheet represents. @alois.schloegl@gmail.com what do you think?

@SparshJ
Copy link
Author

SparshJ commented Feb 1, 2021

Hey! Here's the drive link to the file:
https://drive.google.com/file/d/1WYBXzYVLsp234j4DZwvo1c44UecmH-uR/view?usp=drivesdk

@Teuniz
Copy link

Teuniz commented May 4, 2021

As a workaround, use the tool "Unify resolution", it's in the Tools menu of EDFbrowser.

https://www.teuniz.net/edfbrowser/EDFbrowser%20manual.html#Unify_resolution

@arnodelorme
Copy link
Collaborator

@amisepa you are the EDF expert. Would you mind checking if we can close this report?

@amisepa
Copy link
Contributor

amisepa commented Jul 23, 2021

Sorry, I don't know much on this. @Teuniz In what situation is the physical value not homogenous? Impedance during recording? Glitchy electrode? Can you share how you unify the resolution in EDFBrowser so we could add a similar feature in EEGLAB?
Thanks,
Cedric

@Teuniz
Copy link

Teuniz commented Jul 25, 2021

It appears that the writeeeg function uses the actual peak values found in the signal's samples for pys max and phys min.
This is not as intended by the EDF format.
Pys max and phys min should be the clipping levels of the ADC input and they should be the same for all channels.
If this information is not available, for example when converting from another format, use a safe value for all EEG channels
which is, for example, +3000 uV and -3000 uV.
For example, Nihon Kohden uses +3200 uV and -3200 uV for all EEG channels (which are the actual clipping levels
of their input amplifiers & ADC).
In other words, the phys max and phys min values in the EDF header must NOT be used as an indication of the actual
peak values in that signal in that file.

@amisepa
Copy link
Contributor

amisepa commented Jul 26, 2021

@Teuniz Thank you very much for pointing it out.

@arnodelorme I'll let you decide whether you make this adjustment in writeeeg.

@arnodelorme
Copy link
Collaborator

This is a reasonable assumption, @cll008 would you mind looking at it. Also, test files https://github.com/amisepa/import_EDF

cll008 added a commit to cll008/eeglab that referenced this issue Jul 27, 2021
fix sccn#246 by unifying resolution across channels
Potential issue if one channel is EXG or AUX and range is very different from the rest of the data (e.g. EEG)
@cll008
Copy link
Contributor

cll008 commented Jul 27, 2021

Thanks for reporting the problem @SparshJ and your input, @Teuniz and @amisepa
@arnodelorme asked me to unify based on the most extreme values in the recording. I implemented it, but do you think we should be cautious about EXG or AUX channels that could have different enough scales to cause problems with the rest of the EEG channels?

@cll008
Copy link
Contributor

cll008 commented Jul 27, 2021

Is the ADC range somewhere in the metadata? Looks like writeeeg.m uses a digital range of [-32768 32767] (min is off by 1 compared to FAQ below?) so maybe we can use that for the physical range too...

Excerpt from EDF FAQ

Q4. Are the "digital minimum" and "digital maximum" values hints or strict limits?
The specs say "The digital minimum and maximum of each signal should specify the extreme values that can occur in the data records." Note the word "can". It is not necessary that these values actually DO occur. So take safe values that you know the signal will not exceed, for instance the range of the ADC. Note that "The physical (usually also physiological) minimum and maximum of this signal should correspond to these digital extremes". This correspondence is necessary for assessing gain and offset of the signal.
Q5. Why not always use -32767 for "digital minimum" and +32767 for "digital maximum"?
Export. It is formally correct EDF as long as the purpose (specification of offset and amplification of the signal) is met with sufficient accuracy.

@Teuniz
Copy link

Teuniz commented Jul 28, 2021

EXG and AUX channels usually have different scales/ranges compared to EEG channels.
If available, use the values in the header of the original EDF file that has been used for the input.

@cll008
Copy link
Contributor

cll008 commented Jul 28, 2021

@arnodelorme we can store PhysMax and PhysMin in EEG.etc 33c425c and pass this as an argument to pop_writeeeg and as an option to writeeeg. Thoughts?

@amisepa
Copy link
Contributor

amisepa commented Jul 29, 2021

Funny, I think I just run into this problem randomly trying to export some preprocessed data as .bdf or .edf:

image

image

I think we should hardcode the physMax and PhysMin to the values suggested by Teuniz above (e.g.,, +3200 uV and -3200 uV).
Maybe we can use detection for AUX channels only?

That is when the info is not available in the original EDF file header as suggested.

@arnodelorme
Copy link
Collaborator

@cll008

  1. Test if export is a problem (import, export, reimport) -> this should mess up amplitude
  2. Implement the -32768 to 32768 max limits

@cll008
Copy link
Contributor

cll008 commented Aug 26, 2021

[-32768 32768] is already implemented as digital min/max. I tested [-3200 3200] physical min max as suggested, and found that while EEG channels are okay (0.02% difference from import>export>reimport due to rounding error), ECG channel in test dataset was different by up to 50%. This is because the test file had physical range of [-5353 5353] and the ECG signal was bottomed out to begin with.

@arnodelorme we can either make the physical range larger, we implement storing the header info somewhere.

@amisepa
Copy link
Contributor

amisepa commented Sep 1, 2021

@cll008 I still get this error when importing a .bdf file with biosig toolbox, and try to export it as .bdf

image

Looks like the physical min/max is not allocated to each channel maybe?

There might be solutions to this issues in the conversation shared by Teuniz above. e.g.:
"the datarecord duration is written into the header in plain readable ascii with space for 8 characters, e.g. "0.123456".
This can create rounding errors which results in the samplerate being off.
Second, EDFlib can write max. 1 annotation per datarecord. This tradeoff has been made because the storage space for
annotations needs to beknown in advance, it can not be changed on the fly.
So, lowering the datarecord duration will increase the max. number of annotations that can be written for a given recording duration, increasing it will lower the storage space for annotations.
On a side note, EDFlib writes one annotation channel by default but it can be set to multiple channels if more storage space is needed."

Hope this helps

@cll008
Copy link
Contributor

cll008 commented Sep 2, 2021

Thanks for the input and the summary from the other thread Cedric. The PhysMax and Min are in, but I'm going to check the rest of your test files to see if +-5353 is a more common range than +-3200. PhysDimCode seems to code for the unit, I guess writeeeg never cared about this but it's pretty important. I also wonder if we should change the data length so that the sampling rate doesn't get fudged...

@amisepa
Copy link
Contributor

amisepa commented Sep 2, 2021

Just to clarify:
My error comes from exporting a file (BDF or EDF that I preprocessed in EEGLAB) as .BDF or .EDF, to allow someone to visualize it easily in EDFBrowser. So I think my error is from the exporting functions. But it indicates that there might be something going on with the physMax/Min information somewhere (specifically, the pshyMax/Min is expected to be defined for each channel, which is not the case). That's why I shared it here. I don't know if my test files are the best to base this on as I don't even know what system recorded them.
From that other thread, looks like it's a common issue that might be specific to exporting. And that there might not be a clear solution.

Is it possible to define PhysMax/Min for each channel in writeeg or pop_writeeg? Or do you need to expand dataduration to do that?
See the other thread regarding data duration if you are going to change it, as it doesn't sound very straightforward.

@cll008
Copy link
Contributor

cll008 commented Sep 2, 2021

Yes currently in writeeeg PhysMax and Min can be defined. I've been testing the changes downstream in EDFBrowser too. @arnodelorme decided we want to hardcode this to a reasonable value (unlike the previous scheme which leads to problems by using each signals' max). Really the best is to keep the header info if available but I guess we don't want to add another field to the EEG struct.

@cll008
Copy link
Contributor

cll008 commented Sep 7, 2021

Turns out the test files have inconsistent PhysMax (5353, 5000, 8092, 8719, etc.) so it seems using the max of all channels is a good way to go (barring storing header info on read). I also found a PhysDimCode of 4256+19 codes for uV so I added that to prevent our warning message on import. (PhysDim of uV is already coded for so shouldn't actually cause any issues)

@amisepa
Copy link
Contributor

amisepa commented Sep 10, 2021

Sounds good. FYI: I don't know how these files were recorded and with what system, so I don't know if we should base this decision on them (although helpful). Maybe you can double-check with some more standard files (e.g., biosemi file attached)

6e8691ffd0_eyesclosed.zip

Cedric

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants