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

Use edfio for reading and writing edf files #195

Merged
merged 7 commits into from
Nov 11, 2023
Merged

Conversation

hofaflo
Copy link
Collaborator

@hofaflo hofaflo commented Nov 10, 2023

edfio could replace PyEDFlib and mne for our use cases. We would have to drop support for Python 3.8, but that is in line with the schedule we decided on in #16 anyway.

Copy link
Owner

@cbrnr cbrnr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great! Thank you for this PR, I've only found a typo!

docs/examples.md Outdated Show resolved Hide resolved
@cbrnr
Copy link
Owner

cbrnr commented Nov 10, 2023

Really? We need pooch just to get the SciPy ECG toy data?

@hofaflo
Copy link
Collaborator Author

hofaflo commented Nov 10, 2023

Apparently yes 😬 Do you prefer to bundle an ecg signal with the package instead?

@cbrnr
Copy link
Owner

cbrnr commented Nov 10, 2023

Do you prefer to bundle an ecg signal with the package instead?

Yes. I'd store it as compressed .npz, then it takes up only 163kB. We could store it in sleepecg/data (in analogy to sleepecg/classifiers)?

@cbrnr
Copy link
Owner

cbrnr commented Nov 10, 2023

Maybe we could even add a little function to access the toy data (sleepecg.get_toy_ecg() or something similar)?

@cbrnr
Copy link
Owner

cbrnr commented Nov 10, 2023

Regarding dropping support for Python 3.8, I don't think we can already remove from __future__ import annotations, right? This is still required for Python 3.9, or is it?

@cbrnr
Copy link
Owner

cbrnr commented Nov 10, 2023

Of course the other option is to export the toy data to EDF and add edfio to our base requirements.

@hofaflo
Copy link
Collaborator Author

hofaflo commented Nov 10, 2023

I opted for the .npz file because the .edf would be a bit larger and introduce slight signal deviations.

Ad future imports: A few have become obsolete, but most of them have to stay until 3.10, yes.

@cbrnr
Copy link
Owner

cbrnr commented Nov 10, 2023

Great! I assume nothing has to be done to make sure the data gets added to the wheels, correct? We're still using MANIFEST.in unfortunately.

@cbrnr cbrnr linked an issue Nov 10, 2023 that may be closed by this pull request
@hofaflo
Copy link
Collaborator Author

hofaflo commented Nov 10, 2023

I think graft sleepecg makes sure this is included, yes. As cibuildwheel runs pytest after installing the built wheel, the checks would fail otherwise anyway.

@cbrnr cbrnr merged commit 1c13253 into cbrnr:main Nov 11, 2023
5 checks passed
@cbrnr
Copy link
Owner

cbrnr commented Nov 11, 2023

Thanks @hofaflo!

@cbrnr cbrnr requested review from cbrnr and removed request for cbrnr November 11, 2023 07:58
@hofaflo hofaflo deleted the edfio branch November 11, 2023 12:15
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.

scipy.misc.electrocardiogram has been deprecated
2 participants