-
-
Notifications
You must be signed in to change notification settings - Fork 10.2k
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
API: Add and redefine numpy.bool
[Array API]
#25080
Conversation
Some context from prior discussions: #24306 (comment) The deprecation happened in 1.20 (Jan 2021) and was finalized in NumPy 1.24 (Dec 2022). Is a year enough time for projects to migrate? I'm honestly not sure. Some light github code searches indicate that usages of "np.bool" are mostly in code that hasn't been touched in several years. |
I don't see any problem with putting Either way, a couple more arguments:
|
We should keep that for quite a while, in order to not force people to write if/else stuff depending on the numpy version. In the long run it can go, but there is no hurry I think. |
Agreed, I was trying to say that the migration appears mostly complete in downstream code. I think a year is probably long enough given Ralf's points. |
f546273
to
a9180c0
Compare
numpy.bool
numpy.bool
[Array API]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The main numpy/__init__.pyi
file also needs a bool = bool_
declaration for type checkers to pick up the alias.
This is actually gona be somewhat cumbersome however, as this would result in a naming conflict with the builtin bool
type that can only be resolved via replacing all previous mentions of bool
in the module with an explicit builtins.bool
.
For reference: we've had similar issues before inside the dtype
class due to a dtype.str
/builtins.str
naming conflict (xref #20224)
If we reintroduce |
Right now |
Good point, +1 for making
Slight preference for |
Another +1 for |
One thing about |
I'm a bit confused by this claim; this PR is not putting Perhaps the PR title and release note should be clear that this is not a "reintroduction", but a reallocation of the name to a new thing. |
Yes I understand that, and no it wasn't the only way to go about this. We now effectively made everyone change their code twice, while the vast majority of usage of the old I am pretty sure we discussed undoing the |
@rgommers @seberg I started working on making I already pushed ~900 of them, I still need to fix a few |
numpy.bool
[Array API]numpy.bool
[Array API]
If they're all simple replacements then I think it's fine to do now - but it'd be completely fine to not do it as well. In the end what matters is what we write in the docs I think (and the repr etc. - user-visible stuff); the |
I honestly don't care if we do it now. I was mostly thinking of what the |
0c9b293
to
3ae3d00
Compare
Ok, now reprs for False and True are updated. The PR is ready for a review. |
During the triage call we decided that reprs for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking quite good. Everything works as far as I can tell. I have a couple of small comments inline. My main one is about the type stubs, where you replaced from numpy import _bool
with from numpy import bool
. And as a result have a lot of bool
-> builtins.bool
changes. Those may in some cases be useful anyway for clarity, however wouldn't it be better to not do from numpy import bool
?
Then you'd have to write
NDArray[np.bool]
instead of
NDArray[bool]
but that seems useful anyway.
numpy/_core/fromnumeric.pyi
Outdated
@@ -8,7 +9,7 @@ from numpy import ( | |||
int64, | |||
intp, | |||
float16, | |||
bool_, | |||
bool, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The diff would be a lot smaller if you'd leave out this import. Using the builtin can then stay unchanged. Seems preferable to me to use np.bool
(unless that doesn't work?).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed it to np.bool
.
False_ = bool_(False) | ||
True_ = bool_(True) | ||
False_ = nt.bool(False) | ||
True_ = nt.bool(True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using np.bool
would be clearer; this relies on a weird replacement thing in numerictypes.py
- it does a from builtins import bool
and then overwrites that with:
# Now add the types we've determined to this module
for key in allTypes:
globals()[key] = allTypes[key]
__all__.append(key)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed it, but with np.bool
importing NumPy fails with:
AttributeError: partially initialized module 'numpy' has no attribute 'bool' (most likely due to a circular import)
as numpy.core
module is imported eagerly (__init__.py
imports core/numeric.py
which imports __init__.py
).
I would leave nt.bool
, or import it locally with a different name? WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah okay. In that case, maybe leave it as is, but ensure that there's a test that _core.numerictypes.bool is np.bool
to guard against a future issue there?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure! I added a test for that.
Sure - I can use |
Seems like a good idea to me, but let's ask @BvB93 if he has any preference for these stub files. |
@rgommers I updated the PR - now NumPy's bool is accessed as Could we merge this PR? If we come up with another preference/arrangement for stub files I will adjust it in a separate one. |
The one CI failure is |
That has been a heisenbug (probably in openblas) since forever. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we merge this PR? If we come up with another preference/arrangement for stub files I will adjust it in a separate one.
Agreed. This all looks solid, so let's go with it. Thanks Mateusz & reviewers!
Hi! This PR addresses #11093. It skips `np.bool` and `np.long` replacements as both of these names were reintroduced in NumPy 2.0 with a different meaning (numpy/numpy#24922, numpy/numpy#25080). With this change `NPY001` will no longer conflict with `NPY201`. For projects using NumPy 1.x `np.bool` and `np.long` has been deprecated and removed long time ago, and accessing them yields an informative error message.
Hi @rgommers @ngoldbaum @seberg,
One of the members of Array API is bool. Currently
np.bool_
is present and I think we already had some initial conversations in NEP 52 issues about renaming it tonp.bool
.Would you like to keep both names in the long term? Or should I remove
np.bool_
usages and deprecate it?