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

Implementation and test updates to support NumPy 1.25 #9011

Closed
wants to merge 29 commits into from
Closed

Implementation and test updates to support NumPy 1.25 #9011

wants to merge 29 commits into from

Conversation

apmasell
Copy link
Contributor

NumPy 1.25 has two major changes:

  • allow mixed signed comparisons ufuncs (the qQ->? and Qq->? implementations for >, <, >=, <=, ==, !=); this mode was not supported by Numba and an appropriate implementation is added to match the C implementation in NumPy.
  • the implementation of sin and cos are changed in a way that produces slightly different numeric results. This either lowers the comparison precision where these functions are used in tests or replaces them with another function (mostly exp) to avoid the differences.

NumPy 1.25 has two major changes:

- allow mixed signed comparisons ufuncs (the `qQ->?` and `Qq->?`
  implementations for `>`, `<`, `>=`, `<=`, `==`, `!=`); this mode was not
  supported by Numba and an appropriate implementation is added to match the C
  implementation in NumPy.
- the implementation of `sin` and `cos` are changed in a way that produces
  slightly different numeric results. This either lowers the comparison
  precision where these functions are used in tests or replaces them with
  another function (mostly `exp`) to avoid the differences.
numba/cpython/numbers.py Outdated Show resolved Hide resolved
@stuartarchibald
Copy link
Contributor

Thanks for the patch. RE sin/cos changes, I think this patch numpy/numpy#23925 reverts the implementation that was causing Numba's tests to fail, assuming this is backported to the NumPy 1.25 release branch, Numba will not need to make any related changes.

Changes the implementations to use `select` rather than a branching path for
better optimization potential.
apmasell added 2 commits June 20, 2023 15:24
The corresponding change was removed from NumPy 1.25 so any numeric changes for
sin/cos are also being reverted.
@apmasell apmasell marked this pull request as ready for review June 21, 2023 11:29
@esc esc added this to the Numba 0.58 RC milestone Jun 27, 2023
@@ -30,7 +30,7 @@ conda list
# Use conda-forge for NumPy 1.24 - at the time of writing it is not available
# on the defaults channel.

if [ "${NUMPY}" == "1.24" ]; then
if [ "${NUMPY}" == "1.24" -o "${NUMPY}" == "1.25" ]; then
Copy link
Member

Choose a reason for hiding this comment

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

I think conda-forge is only needed for 1.25 these days (1.24 is in the default channels).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This raises a bit of a concern because we've been using conda-forge packages until now for 1.24. Should we switch this off for 1.25 once Anaconda packages are ready?

@@ -30,7 +30,7 @@ conda list
# Use conda-forge for NumPy 1.24 - at the time of writing it is not available
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# Use conda-forge for NumPy 1.24 - at the time of writing it is not available
# Use conda-forge for NumPy 1.25 - at the time of writing it is not available

@@ -16,7 +16,7 @@ set PIP_INSTALL=pip install -q

@rem Use conda-forge for NumPy 1.24 - at the time of writing it is not available
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
@rem Use conda-forge for NumPy 1.24 - at the time of writing it is not available
@rem Use conda-forge for NumPy 1.25 - at the time of writing it is not available

@@ -16,7 +16,7 @@ set PIP_INSTALL=pip install -q

@rem Use conda-forge for NumPy 1.24 - at the time of writing it is not available
@rem on the defaults channel.
if %NUMPY%==1.24 (set NUMPY_CHANNEL_PKG="conda-forge::numpy") else (set NUMPY_CHANNEL_PKG="numpy")
if %NUMPY% GEQ 1.24 (set NUMPY_CHANNEL_PKG="conda-forge::numpy") else (set NUMPY_CHANNEL_PKG="numpy")
Copy link
Member

Choose a reason for hiding this comment

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

I think this should be OK with NP 1.24 in default channels now:

Suggested change
if %NUMPY% GEQ 1.24 (set NUMPY_CHANNEL_PKG="conda-forge::numpy") else (set NUMPY_CHANNEL_PKG="numpy")
if %NUMPY%==1.25 (set NUMPY_CHANNEL_PKG="conda-forge::numpy") else (set NUMPY_CHANNEL_PKG="numpy")

numba/cpython/numbers.py Outdated Show resolved Hide resolved
@@ -361,6 +361,38 @@ def int_ne_impl(context, builder, sig, args):
return impl_ret_untracked(context, builder, sig.return_type, res)


def int_su_cmp(op):
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps:

Suggested change
def int_su_cmp(op):
def int_signed_unsigned_cmp(op):

would make it a bit easier to guess what the function does from the name? (I had to read the description in the implementation to figure out that s = signed and u = unsigned)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No problem. I had it this way to match the other comparison functions.

return impl


def int_us_cmp(op):
Copy link
Member

Choose a reason for hiding this comment

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

Similar to above,

Suggested change
def int_us_cmp(op):
def int_unsigned_signed_cmp(op):

Copy link
Member

@gmarkall gmarkall left a comment

Choose a reason for hiding this comment

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

Things mostly look good from a quick skim of the diff - there are some comments on it.

The main issue I've come across is that building locally with:

python setup.py develop

results in:

$ python runtests.py -m
RuntimeError: The new DType API is currently in an exploratory phase and should NOT be used for production code.  Expect modifications and crashes!  To experiment with the new API you must set `NUMPY_EXPERIMENTAL_DTYPE_API=1` as an environment variable.
Traceback (most recent call last):
  File "/home/gmarkall/numbadev/numba/numba/testing/__init__.py", line 29, in load_testsuite
    suite.addTests(loader.loadTestsFromName(f))
  File "/home/gmarkall/mambaforge/envs/numbanp125/lib/python3.10/unittest/loader.py", line 154, in loadTestsFromName
    module = __import__(module_name)
  File "/home/gmarkall/numbadev/numba/numba/tests/npyufunc/test_gufunc.py", line 5, in <module>
    import numpy.core.umath_tests as ut
  File "/home/gmarkall/mambaforge/envs/numbanp125/lib/python3.10/site-packages/numpy/core/umath_tests.py", line 13, in <module>
    from ._umath_tests import *
RuntimeError: cannot load _umath_tests module.

I think requiring the user to set an environment variable pertaining to NumPy to run the testsuite is a problem - can / should we skip the relevant tests if numpy.core.umath_tests fails to import?

@gmarkall gmarkall added 4 - Waiting on author Waiting for author to respond to review and removed 3 - Ready for Review labels Jun 27, 2023
apmasell added 3 commits June 27, 2023 16:15
Switch `numpy.core.umath_tests.matrix_multiply`, which is private API, with
`numpy.matmul`, which is public.
@@ -142,7 +142,7 @@ def test(argv, **kwds):
""".split() + types.__all__ + errors.__all__


_min_llvmlite_version = (0, 41, 0)
_min_llvmlite_version = (0, 40, 0)
Copy link
Member

Choose a reason for hiding this comment

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

Was this accidentally added?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops. Yes.

@seberg
Copy link
Contributor

seberg commented Jun 28, 2023

RuntimeError: cannot load _umath_tests module.

Odd, conftest.py should set that environment variable so that you don't run into that (although, I will admit that may not be a great solution). Ping me if it remains/seems like a problem, then I will look into it.

@stuartarchibald stuartarchibald added the 4 - Waiting on author Waiting for author to respond to review label Jul 11, 2023
@stuartarchibald
Copy link
Contributor

Fixed and passing on the build farm.

Any chance you could please be more specific, which buildfarm DAG and which build ID? Thanks.

Prior to merge, this needs to pass on the buildfarm DAG numba-np-next (to test 1.25) and also on the numba_yaml DAG (to test this doesn't break anything existing). Public Azure and gpuCI will also need to pass against HEAD of this branch.

Comment on lines +1425 to +1432
if numpy_version < (1, 25):
def test_impl(A):
min_val = np.amin(A)
return A - min_val
else:
def test_impl(A):
min_val = np.min(A)
return A - min_val
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the purpose of this change? Is it an artifact of fixing the test prior to discovering that np.amin and np.min do not alias in 1.25? If so it can probably be reverted?

Copy link
Member

Choose a reason for hiding this comment

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

This needs reverting. It's from 1a1c7c1 and before new overloads were added

Copy link
Contributor Author

Choose a reason for hiding this comment

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

np.min is not available in NumPy 1.22

Copy link
Contributor

Choose a reason for hiding this comment

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

I think min is just an alias of amin in NumPy 1.22.x:

>>> import numpy as np
>>> np.__version__
'1.22.3'
>>> np.min
<function amin at 0xsomeaddress>

I'm also of the view that this change needs reverting, amin is the function being tested originally and should continue to be tested invariant of how NumPy have implemented it.

As requested in review.
@@ -30,6 +30,9 @@ def np_around_unary(val):
def np_round_array(arr, decimals, out):
np.round(arr, decimals, out)

def np_round__array(arr, decimals, out):
Copy link
Member

Choose a reason for hiding this comment

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

What is the double-underscore variant of this function and test for? It looks identical to the version above - am I missing something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's round and round_ in NumPy, so, I'm trying to continue the pattern.

Copy link
Member

Choose a reason for hiding this comment

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

The function calls np.round though, rather than np.round_.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ooops.

@gmarkall
Copy link
Member

gpuci run tests

@gmarkall
Copy link
Member

gpuci run tests

1 similar comment
@gmarkall
Copy link
Member

gpuci run tests

NumPy 1.25 packages are not provided for Python < 3.10 on the default
conda channel, so we need to use a newer version.
@gmarkall
Copy link
Member

gpuci run tests

There isn't a suitable RAPIDS image with 3.10 that keeps the versions of
all other components the same, so instead we'll keep the 3.8 image and
create a 3.10 environment in it.
@gmarkall
Copy link
Member

gpuci run tests

@gmarkall
Copy link
Member

@apmasell I hope you don't mind - I fixed the conflicts with the gpuCI setup and also fixed an issue with the config related to the Python version in conjunction with NumPy, so that this could be tested on gpuCI (rather than asking you to make changes and waiting).

As titled.
@apmasell
Copy link
Contributor Author

No problem with the changes for gpuCI.

@apmasell apmasell closed this Jul 28, 2023
esc added a commit that referenced this pull request Aug 1, 2023
NumPy 1.25 support (PR #9011) continued
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4 - Waiting on author Waiting for author to respond to review BuildFarm Passed For PRs that have been through the buildfarm and passed highpriority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants