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

Test for compatibility with Numpy prerelease (2.0 is coming this month) #3950

Closed
3 tasks
Zac-HD opened this issue Apr 11, 2024 · 9 comments · Fixed by #3955
Closed
3 tasks

Test for compatibility with Numpy prerelease (2.0 is coming this month) #3950

Zac-HD opened this issue Apr 11, 2024 · 9 comments · Fixed by #3955
Labels
interop how to play nicely with other packages tests/build/CI about testing or deployment *of* Hypothesis

Comments

@Zac-HD
Copy link
Member

Zac-HD commented Apr 11, 2024

pydata/xarray#8844 (comment) points out that this test is failing on the upcoming Numpy 2.0, and it looks like we're incompatible.

Specifically, hypothesis.extra.array_api.make_strategies_namespace() eventually calls next_up() on an instance of numpy.float32, and makes the failing asserting that this is an instance of builtin float.

  • Add a numpy-prerelease build to our CI system; we can allow failures (as for pyodide) but it's good to know
  • If that doesn't reproduce this problem, add a test which does
  • Fix the incompatibility, so that we'll work out of the box when Numpy 2.0 is released
@Zac-HD Zac-HD added tests/build/CI about testing or deployment *of* Hypothesis interop how to play nicely with other packages labels Apr 11, 2024
@keewis
Copy link
Contributor

keewis commented Apr 12, 2024

potential test case:

from hypothesis import given
from hypothesis.extra.array_api import make_strategies_namespace
import numpy as np

try:
    import numpy.array_api
    xps = make_strategies_namespace(numpy.array_api)
except ImportError:
    xps = make_strategies_namespace(np)

dtypes = xps.floating_dtypes()
shapes = xps.array_shapes()
arrays = xps.arrays(dtype=dtypes, shape=shapes)


@given(arrays)
def test_array_strategy(arr):
    # or even just `pass`
    assert hasattr(arr, "dtype")

as pointed out by @seberg, the cause might be the changed casting rules in numpy 2.0, where before float32(3) + 0.0 would be casted to float64 (and thus pass the check) while now the result is a float32.

@keewis
Copy link
Contributor

keewis commented Apr 21, 2024

I believe the issue is that the types in the return value of np.finfo changed:

numpy<2:

finfo = np.finfo(np.float32)
type(finfo.smallest_normal)  # builtin float 

numpy>=2:

finfo = np.finfo(np.float32)
type(finfo.smallest_normal)  # float32

One way to resolve this would be to simply pass finfo.smallest_normal through the builtin float constructor (another would be finfo.smallest_normal.item(), but that would only work for the numpy floats.

Edit: or maybe not? Let me check again.
Edit2: the issue is actually this:

import numpy.array_api as xp
import numpy as np

finfo = xp.finfo(np.float32)
type(finfo.smallest_normal)  # float

and the same with array_api_strict. So while numpy.finfo has always returned numbers of the same dtype, the array API namespaces (both array_api_strict and numpy.array_api) return builtin floats.

@keewis
Copy link
Contributor

keewis commented Apr 21, 2024

@rgommers, @seberg (or anyone else who knows more about this than me), is that inconsistency something we should expect? Or has this simply been missed when working on the compatibility of finfo in the main numpy namespace?

@rgommers
Copy link

I'm not sure that this was on purpose - it could be, but I don't remember. And while float64 instances ducktype with builtin float, float32 doesn't to the same extent:

>>> isinstance(np.float64(1.5), float)
True
>>> isinstance(np.float32(1.5), float)
False

There were a lot of PRs that touched on finfo. numpy/numpy#23088 is the one that jumps out most to me.

@seberg
Copy link

seberg commented Apr 21, 2024

This is just indirect promotion changes, but looks fine to me. Finfo might have more precision than builtin float anyway.

@keewis
Copy link
Contributor

keewis commented Apr 21, 2024

to be clear (my edits may have been confusing): numpy.finfo never changed, it has always had the fields of the finfo struct with the same dtype as the input.

However, both numpy.array_api (in numpy<2) and array_api_strict return the fields as builtin float. So my question was: does the Array API require builtin float values for the fields of the finfo struct, or is any floating-point dtype fine? In other words, is numpy.finfo compliant or not (with the direct consequence here: do we need to adapt the code in hypothesis, or can we expect numpy to change this before the final release)?

Finfo might have more precision than builtin float anyway.

Apparently builtin float always has 64 bits (C double), so I would have expected the inverse? At least if we don't consider float128 (C long double).

@seberg
Copy link

seberg commented Apr 22, 2024

Ah thanks for the clarification, I was indeed thinking you showed that NumPy changed, and not the strict array API namespace.
For that namespace, the change may be unintentional, although I think it may be for the better also there: I might suggest to just convert to float here with a comment that you don't support long-doubles anyway?

(I don't recall the discussion but it doesn't seem ideal to specify a float return, since higher precision floats cannot adhere to it. That would mean that a package which wants to adhere to it but also supports higher precision floats of whatever kind would be forced to export inconsistent types for finfo.)

@keewis
Copy link
Contributor

keewis commented Apr 22, 2024

(no changes to the array API namespaces either, they also always returned the same thing – but something different than numpy.finfo. The issue was that the main numpy namespace is now considered to be array API compliant)

I might suggest to just convert to float here with a comment that you don't support long-doubles anyway?

long double support probably is something that should be considered in the future, but for now I agree, casting to float doesn't change behavior.

@rgommers
Copy link

I was indeed thinking you showed that NumPy changed

Same here. Okay, less concerned then:)

(I don't recall the discussion but it doesn't seem ideal to specify a float return, since higher precision floats cannot adhere to it. That would mean that a package which wants to adhere to it but also supports higher precision floats of whatever kind would be forced to export inconsistent types for finfo.)

There is no way to do anything different aside from changing from a scalar to a 0-D array. Which would be a breaking change, that doesn't seem warranted. float was simply the common denominator between the various libraries.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
interop how to play nicely with other packages tests/build/CI about testing or deployment *of* Hypothesis
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants