Skip to content

Map EDF digital endpoints to their physical endpoints - #1920

Open
antonsoo wants to merge 2 commits into
NeuralEnsemble:masterfrom
antonsoo:fix/edf-physical-calibration
Open

antonsoo wants to merge 2 commits into
NeuralEnsemble:masterfrom
antonsoo:fix/edf-physical-calibration

Conversation

@antonsoo

Copy link
Copy Markdown

EDFRawIO divides the physical range by digital_max - digital_min + 1, introducing a gain and offset error. The EDF specification defines corresponding physical and digital endpoints, so the denominator needs the difference between those endpoints. For an identity range of -8092 to 8092, a stored value of -5 currently reads as approximately -5.49966 rather than -5.

Remove the extra interval. Add generated EDF regressions for identity, asymmetric and full16bit calibration, both output dtypes and reordered/subset channels. Update the existing EDFIO comparison to use PyEDFlib physical reads rather than repeating the old denominator.

Validation: 632 native core/calibration tests and six subtests pass; Black passes. Thirteen external-fixture tests are skipped because the GIN fixture downloads return HTTP403. The existing EDFIO comparison method also passes on an explicitly generated five-channel fixture. Recorded EEGMMIDB S001/R04 samples now agree exactly with independent EDF decoding across 16 raw/eager/lazy conversions; raw integers, timestamps and channel ordering stay unchanged.

The contributor-list commit is shared with #1919. The two runtime fixes are independent.

Use the difference between the digital extrema for calibration. Counting both endpoints as an additional interval introduces a systematic gain and offset error. Test real generated EDF endpoint samples and compare physical IO reads against pyedflib instead of repeating the incorrect denominator.
@zm711
zm711 requested a review from JuliaSprenger October 18, 2026 16:52
@zm711

zm711 commented Oct 18, 2026

Copy link
Copy Markdown
Contributor

@JuliaSprenger, it appears you wrote the initial implementation here and I don't know this reader at all, so could you look over this?

This branch has not been deployed

No deployments
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.

2 participants