beacon-biosignals / EDF.jl

Read and write EDF files in Julia
MIT License
18 stars 5 forks source link

RFC: More principled errors #75

Open ericphanson opened 12 months ago

ericphanson commented 12 months ago

closes #74

This captures my thoughts on what I think EDF.jl should do:

This should probably only be merged/released after OndaEDF reliably chooses "good" encoding parameters that don't incur too much roundoff (xref https://github.com/beacon-biosignals/OndaEDF.jl/issues/90)), and this should likely be a breaking release of EDF.jl.

TODO:

codecov[bot] commented 12 months ago

Codecov Report

Merging #75 (f6fbea2) into main (2eef49f) will increase coverage by 0.48%. The diff coverage is 97.82%.

@@            Coverage Diff             @@
##             main      #75      +/-   ##
==========================================
+ Coverage   95.59%   96.07%   +0.48%     
==========================================
  Files           4        4              
  Lines         295      306      +11     
==========================================
+ Hits          282      294      +12     
+ Misses         13       12       -1     
Files Coverage Δ
src/types.jl 89.65% <ø> (-1.78%) :arrow_down:
src/write.jl 96.94% <97.82%> (+1.33%) :arrow_up:

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

ericphanson commented 12 months ago

BTW, to choose the tolerance, given the 4 encoding parameters as Float64, we can actually compute the error in roundtripped sample (after encoding & decoding with rounded values) and choose a tolerance in data-space rather than a tolerance in encoding-parameter-space (which would be actually "principled"). For example, we could have a principle that the additional maximal error incurred by "rounding of encoding parameters" should be less than half of the error incurred by quantization in the first place. Given digital min/max, physical min/max, we should be able to actually compute both errors analytically. This would require evaluating all 4 together, rather than each separately.

ericphanson commented 11 months ago

@ararslan would love to know your thoughts here!