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

Run tests against NumPy 2 #1234

Closed
tomwhite opened this issue Jun 24, 2024 · 3 comments · Fixed by #1237
Closed

Run tests against NumPy 2 #1234

tomwhite opened this issue Jun 24, 2024 · 3 comments · Fixed by #1237
Labels
upstream Used when our build breaks due to upstream changes

Comments

@tomwhite
Copy link
Collaborator

Tim made some fixes in #1228, but there are still some more

@tomwhite tomwhite added the upstream Used when our build breaks due to upstream changes label Jun 24, 2024
@tomwhite
Copy link
Collaborator Author

... the tests against scikit-allel are still failing. Maybe time to add the scikit-allel test data and drop the dependency?

Originally posted by @timothymillar in #1228 (comment)

I didn't see this before, but I agree - it looks like scikit-allel won't work against NumPy 2 since its cython code is incompatible.

We only use it in the tests for comparing our methods implementations:

  • test_ld.py - we could drop it from here since it's not testing anything non-trivial.
  • test_pca.py
  • test_popgen.py - we could compare to tskit instead of scikit-allel (we already do in a few places).
  • test_preprocessing.py
  • test_window.py

As @timothymillar said, we could export some test data from scikit-allel for the few remaining cases where we want to check the output of sgkit.

@tomwhite
Copy link
Collaborator Author

Another failing case I found was in the VCF writer code.

================================================================================= FAILURES ==================================================================================
__________________________________________________________________________ test_ftoa[0.0005-0.001] __________________________________________________________________________

f = np.float32(0.0005), a = '0.001'

    @pytest.mark.parametrize(
        "f, a",
        [
            (0.0, "0"),
            (0.0001, "0"),
            (0.0005, "0.001"),
            (0.3, "0.3"),
            (0.32, "0.32"),
            (0.329, "0.329"),
            (0.3217, "0.322"),
            (8.0, "8"),
            (8.0001, "8"),
            (8.3, "8.3"),
            (8.32, "8.32"),
            (8.329, "8.329"),
            (8.3217, "8.322"),
            (443.998, "443.998"),
            (1028.0, "1028"),
            (1028.0001, "1028"),
            (1028.3, "1028.3"),
            (1028.32, "1028.32"),
            (1028.329, "1028.329"),
            (1028.3217, "1028.322"),
            (np.nan, "nan"),
            (np.inf, "inf"),
        ],
    )
    def test_ftoa(f, a):
        f = np.array([f], dtype=np.float32)[0]
        buf = np.empty(FLOAT32_BUF_SIZE, dtype=np.uint8)
    
        p = ftoa(buf, 0, f)
>       assert byte_buf_to_str(buf[:p]) == a
E       AssertionError: assert '0' == '0.001'
E         
E         - 0.001
E         + 0

sgkit/tests/io/vcf/test_vcf_writer_utils.py:96: AssertionError
========================================================================== short test summary info ==========================================================================
FAILED sgkit/tests/io/vcf/test_vcf_writer_utils.py::test_ftoa[0.0005-0.001] - AssertionError: assert '0' == '0.001'

I think this was actually passing by accident before. NumPy will round towards the nearest even value, so in this case it would be 0.000, not 0.001. However, due to what I think were changes in NumPy around value-based promotion (https://numpy.org/neps/nep-0050-scalar-promotion.html) this test fails in NumPy 2, while it passed in NumPy 1. Also, in Numba it passes - when using NumPy 1 or 2, since Numba has not yet caught up on that aspect of NumPy 2 (i.e. it is still following NumPy 1 behaviour).

So I think it's best to drop the test for that case, since it is arguably incorrect anyway (0.0005 should round to 0).

@tomwhite tomwhite mentioned this issue Jun 25, 2024
4 tasks
@tomwhite
Copy link
Collaborator Author

tomwhite commented Sep 2, 2024

I noticed that scikit-allel is now compatible with NumPy 2, which makes this a lot easier! (Thanks @alimanfoo for that!)

I have updated #1237.

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
upstream Used when our build breaks due to upstream changes
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant