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

Converted assert of phys_min and phys_max into warnings #266

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

TrianecT-Wouter
Copy link

Converted assert's into warnings.

Reason:
in many cases, storing an edf file fails because of roundoff errors: e.g. phys_min -3565.82 vs. signal_min 3565.8200000000000006 will fail in write_edf (but shouldn't)

Converted assert's into warnings. Reason:

in some cases, storing an edf file fails because of roundoff errors:
e.g. `phys_min -3565.82 vs. signal_min 3565.8200000000000006` will fail in write_edf (but shouldn't)
@skjerns
Copy link
Collaborator

skjerns commented Dec 16, 2024

nice! It seems like it was originally intended to be a warning, so good fix.

However, could you also adapt the test suite? It seems to expect an assertion error here when we no longer give this. Maybe check for the actualy values that are now being read? that would be great!

@TrianecT-Wouter
Copy link
Author

Didn't see that before. Will check.

@TrianecT-Wouter
Copy link
Author

Good tips @skjerns. I attempted to implement the tests as well and found some new edge cases which should still throw an error.

Workflow is now as follows:

  1. check if the difference is large (using accuracy based on the fact that edf uses 16 bits for the range from -physmin / +physmax)
  2. Yes: perform assertion
  3. No: throw warning

Also reflected in tests.

To enable building the python package, I had to remove to references to distools (which is deprecated; see #267)

This also fixes #267 in the sense that you can still build the pypi package using python 3.12.

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