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

Be less strict about prefilter fields #114

Merged
merged 2 commits into from
Jun 27, 2021
Merged

Conversation

wmvanvliet
Copy link
Contributor

I'm trying to load EDF file from this dataset: https://physionet.org/content/eegmmidb
In these files, prefilter settings are present in both the normal fields and also in the EDF+ annotations.
For some reason, it was decided that pyedflib should flat out refuse to read such files?
I think that is a bit harsch.

@codecov
Copy link

codecov bot commented Feb 26, 2021

Codecov Report

Merging #114 (40d8da6) into master (bd803bd) will increase coverage by 0.03%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #114      +/-   ##
==========================================
+ Coverage   67.84%   67.87%   +0.03%     
==========================================
  Files           7        7              
  Lines        3623     3614       -9     
  Branches      166      166              
==========================================
- Hits         2458     2453       -5     
+ Misses       1109     1105       -4     
  Partials       56       56              
Impacted Files Coverage Δ
pyedflib/_extensions/c/edflib.c 66.82% <ø> (+0.03%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bd803bd...40d8da6. Read the comment docs.

@skjerns
Copy link
Collaborator

skjerns commented Feb 28, 2021

I understand the annoyance with being so strict about keeping to the EDF specifications. The library we wrap edflib is very strict with regards to these. Even though I would be kind of fine to just read these incorrectly formatted files anyway, I also see why being lose with standards will not help in the long run.

In this particular case: What would happen if prefilter fields have been set to both positions? Which one would be reported?

@wmvanvliet
Copy link
Contributor Author

In the original code, it would crash. With this PR, the first would be taken.

@skjerns skjerns merged commit 87dbb05 into holgern:master Jun 27, 2021
@skjerns
Copy link
Collaborator

skjerns commented Jun 27, 2021

fixes #120

thanks @wmvanvliet ! I decided it is indeed a bit harsh to just refuse to load them, and as others seem to have the same problem, I merged your suggestions.

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