ENH: Update numpy exceptions imports #16972
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
mtsokol
force-pushed
the
update-np-exceptions-imports
branch
from
69adc51 to
736a95a
Compare
google-ml-butler
bot
added
kokoro:force-run
pull ready
hawkinsp
left a comment
hawkinsp
left a comment
There was a problem hiding this comment.
Looks like there are some test failures from using numpy_version when numpy_version() should be used.
There was a problem hiding this comment.
Looks good - thanks for looking into this!
One suggestion – rather than the version check, what if we used a try...except?
try:
# numpy 1.25.0 or newer
NumpyComplexWarning = np.exceptions.ComplexWarning
except AttributeError:
# legacy numpy
NumpyComplexWarning = np.ComplexWarningVersion checks are fine for tests, but I'd prefer to avoid them in package import paths – e.g. imagine a user who has some custom numpy build with a version string that we don't anticipate, and then import jax might fail on trying to cast the version to a tuple of ints.
jax/_src/util.py
Outdated
| if numpy_version() >= (1, 25, 0): | ||
| ComplexWarning = np.exceptions.ComplexWarning | ||
| else: | ||
| ComplexWarning = np.ComplexWarning |
There was a problem hiding this comment.
What if we named this NumpyComplexWarning from the start to avoid the need to rename the imports?
|
Sure! I will add these changes. For the version check I followed Also, I wanted to want to keep you in the loop on the progress of NEP 52, which will influence NumPy's API. In this issue there's an ongoing discussion about changes in the main namespace: numpy/numpy#24306 (comment) (in the details of this comment there is a |
|
Thanks for pointing to NEP 52. Everything looks pretty straightforward, but one thing I'm confused about is the dtype sized alias thing. If I am referencing |
Current status is that sized aliases are kept in the same place and only "other aliases" mentioned in NumPy docs are being removed. |
Yes, I moved back to a version where |
jakevdp
left a comment
jakevdp
left a comment
There was a problem hiding this comment.
Looks good - one more comment below.
After that, please also squash your changes into a single commit (see https://jax.readthedocs.io/en/latest/contributing.html#single-change-commits-and-pull-requests)
mtsokol
force-pushed
the
update-np-exceptions-imports
branch
2 times, most recently
from
e20b0e0 to
fa4342a
Compare
mtsokol
force-pushed
the
update-np-exceptions-imports
branch
from
f3b17ee to
d183a2c
Compare
4 participants
Hi!
Due to NumPy's main namespace being changed in numpy/numpy#24316, here I update warning imports. Please share your feedback!