-
-
Notifications
You must be signed in to change notification settings - Fork 10.8k
BUG: Numpy scalar types sometimes have the same name #10151
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
Conversation
aaa43af
to
cec5248
Compare
cec5248
to
077ac4f
Compare
077ac4f
to
1658397
Compare
110e934
to
2e57b32
Compare
Finally passing. There were a bunch of places that actually relied upon the name being what it was. |
2e57b32
to
8e15d05
Compare
8e15d05
to
ce1a079
Compare
Rebased on #11328, now a very contained diff |
This looks like a nice solution; one question, though: is the definition such that if one does
one always gets a dtype with a repr that is Indeed, could you add a test that explicitly checks this? |
@mhvk: That's not the right constraint. The requirement is that I think that what you ask for happens anyway, but i don't think testing it is in scope for this patch. |
My comment was only that this particular aspect should not change; as I don't have easy access to a 32-bit machine, though, I cannot test it... |
I don't know if I agree with that comment. There are two questions here:
My claim is that this patch does not change the answer to the first question, and that its goal is to reduce confusion by making the answer to the second question match the first one. If those answers are As it happens, I think that the first answer is always true anyway, since the default integer type is a C |
xref #12179 which is also about cleanups around |
Yep, I keep running into problems that need solving before I can get this one working |
4e127e6
to
c848326
Compare
@eric-wieser What do you want to do with this? |
Needs rebase |
This seems a WIP, so pushing off to 1.17. |
A Numpy string formatting bug causes this doctest to fail on certain 32-bit platforms (e.g. armhf). The bug is evident in the following example: >>> np.arange(8) array([0, 1, 2, 3, 4, 5, 6, 7]) >>> 1 << np.arange(8) array([ 1, 2, 4, 8, 16, 32, 64, 128], dtype=int32) The `dtype=int32` should be supressed in both cases, but it is not. This might get fixed upstream in Numpy 1.17. See numpy/numpy#9799, numpy/numpy#10151.
c848326
to
703cb31
Compare
Tests passing |
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 am good with giving this a shot, I cannot really think of a reasonable way to write code that is broken by this...
This removes a test that enforced the opposite - dtype.name is documented as being a bitname, but it is exactly this property that causes confusion when applied to __name__ - so we should not expect them to be equal. Fixes numpygh-9799
703cb31
to
fa09f5e
Compare
Updated with a tweaked comment and better tests |
------------------------------------------------------------ | ||
On any given platform, two of ``np.intc``, ``np.int_``, and ``np.longlong`` | ||
would previously appear indistinguishable through their ``repr``, despite | ||
having different properties when wrapped into ``dtype``s. |
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.
nit: the s
right after the double back-ticks sometimes causes problems when rendering, but I am not sure what the actual logic is. Hopefully can be caught when rendering the release note
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.
You're right, needs to be ``dtype``\ s
Thanks, let's give this a shot. I hope there are no work-arounds in user code that this breaks. |
Fixes gh-9799.
This breaks(no longer true)np.core.numerictypes.bitname
on integer types, but perhaps it's private enough that we don't care.Tested on 64-bit windows locally.
Before:
After:
If we want to "fix" the dtype output too, then it probably makes sense to wait for #10602