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

Deprecate sample_rate #268

Merged
merged 6 commits into from
Jan 21, 2025
Merged

Deprecate sample_rate #268

merged 6 commits into from
Jan 21, 2025

Conversation

skjerns
Copy link
Collaborator

@skjerns skjerns commented Jan 18, 2025

As discussed in #247 , I deprecated sample_rate and replaced all of its occurences with sample_frequency.

Frequency is usually measured in Hz, which is defined per second and is most consistent with other libraries (e.g. mne).

Summary

  • There are errors raised when users try to use sample_rate - this is to ensure everyone finds out about the change without googeling
  • I added a new routine to calculate record_duration - it should find an optimal value if one exists and let users not worry about the sample_rate as it is defined in edflib but to simply sey sample_frequency and be done
  • It's still possible to overwrite the value of record_duration if ppl actually need to for obscure reasons

Additionally, needed to deprecate 3.7 for the test suite as it's not available for ubuntu24

fixes #198
closes #247

@skjerns
Copy link
Collaborator Author

skjerns commented Jan 18, 2025

@cbrnr @cfranklin11 @LucaCerina @raphaelvallat @DimitriPapadopoulos if any of you has time to have a look, that would be great :)

Copy link
Contributor

@raphaelvallat raphaelvallat left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@LucaCerina
Copy link
Contributor

Good also for me

Copy link
Contributor

@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.

Just a few minor suggestions, otherwise LGTM (although I didn't check the record duration calculation since I don't have the time at the moment).

.github/workflows/tests.yml Outdated Show resolved Hide resolved
pyedflib/_extensions/_pyedflib.pyx Outdated Show resolved Hide resolved
pyedflib/edfwriter.py Outdated Show resolved Hide resolved
pyedflib/edfwriter.py Outdated Show resolved Hide resolved
@skjerns
Copy link
Collaborator Author

skjerns commented Jan 21, 2025

Found something strange: Even though the record_duration can be 8 ASCII chars according to the definition, it truncates after 7 in our library.

f.setDatarecordDuration(0.123456)
...
AssertionError: read 0.12345 != expected 0.123456

In the raw header there is an empty space

image

this issue might be related #242. I suggest fixing this in a future PR.

@skjerns
Copy link
Collaborator Author

skjerns commented Jan 21, 2025

Given that the the requested changes were minor, I'll merge this one :) thanks for reviewing!

@skjerns skjerns merged commit b5df218 into master Jan 21, 2025
32 of 33 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants