MIT-LCP / wfdb-python

Native Python WFDB package
MIT License
730 stars 298 forks source link

bug-fix: Numpy ValueError when cheking empty list equality #459

Closed ajadczaksunriselabs closed 1 year ago

ajadczaksunriselabs commented 1 year ago

Using the equality operator with an empty list will result in the following error.

ValueError: operands could not be broadcast together with shapes (28,) (0,)

Using len() and inverting the logic avoids this issue.

ajadczaksunriselabs commented 1 year ago

No idea what is going on with the test failures here, but it doesn't seem to be entirely related to the changes I made... I get test failures on main when executing the following steps:

  1. Created and activated a fresh python 3.11 venv
  2. Cloned the github.com/MIT-LCP/wfdb-python repo
  3. Navigated to the repo base director and ran pip install ".[dev]"
  4. Navigated to tests and ran pytest (I didn't see any specific instructions so this seemed like the logical thing to do)

image

Most of the failures seem to be related to missing test data image

Maybe I missed a test in the test setup? If there are additional instructions for unit testing let me know and I'll try to figure out which of the failures are related to this pull request.

tompollard commented 1 year ago

I haven't looked into the errors that you're seeing, but my guess is this is either a path issue or an operating system issue.

Typically I would expect to run pytest from the root of a project. Pytest will then iterate folders looking for tests. You may find that running pytest from the root of your project fixes the issue (because the files are now on the correct path).

If the approach above doesn't work, then it's possible that the tests are set up to run on a unix style operating system. If so, we would need to modify them to work on (Windows?).

ajadczaksunriselabs commented 1 year ago

running pytest from the base directory does fix the file not found issues I was encountering and I'm getting much better results (or at least results that match python3.9 on debian on WSL). image image

The failures are identical between windows and debian (WSL) now image

image

which is encouraging.

If I apply this patch and run pytest the correct way all test pass (with warnings) on both windows and debian.

image

image

So now I'm at a loss to why the CI is blowing up.

bemoody commented 1 year ago

Thanks for your help!

So now I'm at a loss to why the CI is blowing up.

Yeah, the GitHub workflow error reporting is not great. In this case it looks like there is something severely broken in GitHub's version of Python 3.7 on MacOS ("No module named '_bz2'"). Perhaps relatedly, CPython 3.7 is now officially at its end of life.

At the same time, this numpy issue needs to be fixed in order for the other CI tests to pass.

Could you please run:

git fetch --all
git merge 07da4f48dad5cfc07420a061db4fd341a4e4a089