Skip to content

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

Merged
merged 1 commit into from
Sep 12, 2019

Conversation

eric-wieser
Copy link
Member

@eric-wieser eric-wieser commented Dec 2, 2017

Fixes gh-9799.

This breaks np.core.numerictypes.bitname on integer types, but perhaps it's private enough that we don't care. (no longer true)

Tested on 64-bit windows locally.

Before:

>>> np.dtype(np.int_), np.int_
(dtype('int32'), numpy.int32)

>>> np.dtype(np.intc), np.intc
(dtype('int32'), numpy.int32)

>>> np.dtype(np.double), np.double
(dtype('float64'), numpy.float64)

>>> np.dtype(np.longdouble), np.longdouble
(dtype('float64'), numpy.float64)

After:

>>> np.dtype(np.int_), np.int_
(dtype('int32'), numpy.int32)

>>> np.dtype(np.intc), np.intc
(dtype('int32'), numpy.intc)

>>> np.dtype(np.double), np.double
(dtype('float64'), numpy.float64)

>>> np.dtype(np.longdouble), np.longdouble
(dtype('float64'), numpy.longdouble)

If we want to "fix" the dtype output too, then it probably makes sense to wait for #10602

@eric-wieser eric-wieser force-pushed the integer-type-__name__ branch from cec5248 to 077ac4f Compare January 5, 2018 16:15
@eric-wieser eric-wieser force-pushed the integer-type-__name__ branch from 077ac4f to 1658397 Compare January 7, 2018 00:28
@eric-wieser eric-wieser force-pushed the integer-type-__name__ branch 4 times, most recently from 110e934 to 2e57b32 Compare January 10, 2018 03:46
@eric-wieser
Copy link
Member Author

Finally passing. There were a bunch of places that actually relied upon the name being what it was.

@eric-wieser
Copy link
Member Author

Rebased on #11328, now a very contained diff

@mhvk
Copy link
Contributor

mhvk commented Jun 26, 2018

This looks like a nice solution; one question, though: is the definition such that if one does

np.array(1, dtype=int).dtype

one always gets a dtype with a repr that is dtype('int32') or dtype('int64')? I ask because for the "default" python int one should definitely get the form that has the number of bits.

Indeed, could you add a test that explicitly checks this?

@eric-wieser
Copy link
Member Author

eric-wieser commented Jun 26, 2018

@mhvk: That's not the right constraint. The requirement is that np.int64.__name__ == 'int64'. We can't change what np.int64 points to now without risking breaking downstream users

I think that what you ask for happens anyway, but i don't think testing it is in scope for this patch.

@mhvk
Copy link
Contributor

mhvk commented Jun 26, 2018

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...

@eric-wieser
Copy link
Member Author

eric-wieser commented Jun 26, 2018

I don't know if I agree with that comment.

There are two questions here:

  1. Is np.array(1).dtype.type is np.int64?
  2. Is np.array(1).dtype.type.__name__ == 'int64'?

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 False, that's unfortunate, but changing (1) has compatibility ramifications, and changing (2) does not.

As it happens, I think that the first answer is always true anyway, since the default integer type is a C long, which gets first choice of name.

@mattip
Copy link
Member

mattip commented Oct 16, 2018

xref #12179 which is also about cleanups around dtype.__repr__

@eric-wieser
Copy link
Member Author

Yep, I keep running into problems that need solving before I can get this one working

@charris
Copy link
Member

charris commented Nov 13, 2018

@eric-wieser What do you want to do with this?

@charris
Copy link
Member

charris commented Nov 14, 2018

Needs rebase

@charris
Copy link
Member

charris commented Nov 14, 2018

This seems a WIP, so pushing off to 1.17.

@charris charris modified the milestones: 1.16.0 release, 1.17.0 release Nov 14, 2018
lpsinger added a commit to lpsinger/healpy that referenced this pull request Jan 17, 2019
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.
@charris charris removed this from the 1.17.0 release milestone May 11, 2019
@eric-wieser
Copy link
Member Author

Tests passing

Copy link
Member

@seberg seberg left a 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
@eric-wieser
Copy link
Member Author

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.
Copy link
Member

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

Copy link
Member Author

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

@mattip mattip merged commit 2f81858 into numpy:master Sep 12, 2019
@mattip
Copy link
Member

mattip commented Sep 12, 2019

Thanks, let's give this a shot. I hope there are no work-arounds in user code that this breaks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BUG: np.int_ and np.intc / np.longlong have the same __name__ but are different
5 participants