Skip to content

DEP: Remove deprecated numeric types and deprecate remaining#16554

Merged
mattip merged 5 commits intonumpy:masterfrom
seberg:deprecate-numerictypes
Jun 10, 2020
Merged

DEP: Remove deprecated numeric types and deprecate remaining#16554
mattip merged 5 commits intonumpy:masterfrom
seberg:deprecate-numerictypes

Conversation

@seberg
Copy link
Copy Markdown
Member

@seberg seberg commented Jun 9, 2020

This removes the types from usage for np.dtype(), since the
two are closely related, also finishes the deprecation of
typeNA and sctypeNA (thus removing them from the public API).

The deprecation for the numeric types was only shown for
np.dtype("Complex64"), etc. however, here they are also
removed from np.typeDict and np.sctypeDict.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why specifically these ones?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I looked at the diff between before and after these changes. And those were all the remaining ones, so I figured we might as well deprecate them as well... It seems weird to me to get rid of Timedelta, but keep Datetime.

@seberg
Copy link
Copy Markdown
Member Author

seberg commented Jun 9, 2020

For everyone, the difference between typeDict before and after this branch is:

{'Bool',  'Complex128', 'Complex32', 'Complex64', 'Float128', 'Float16', 'Float32', 'Float64',
 'Int16', 'Int32', 'Int64', 'Int8', 'Object0', 'Timedelta64', 'UInt16', 'UInt32', 'UInt64', 'UInt8',
 'Void0'}

You see that Datetime64, etc. are the ones I explicitly added back. And on this branch, these are the only (more than single characters or "M8") type codes which start with an upper case letter:

[...
 'Bytes0',
 'Datetime64',
 'Str0',
 'Uint64']

which is the ones I added to be deprecated now.

@seberg seberg force-pushed the deprecate-numerictypes branch from 79b6cbc to b162875 Compare June 9, 2020 20:32
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This if is weird, but it failed tests and I think it is correct... if anyone has a 32bit linux or maybe windows to see that Uint64 was never included in np.typeDict on old versions, that would be good.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It now passes tests, is this comment outdated?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

No, I worked around it. But, I now realized on windows we probably had a stray Uint32 and that will be missing now...

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

np.typeDict has UInt64 on 1.16, windows 32 bit:

>>> sys.version
'2.7.18 (v2.7.18:8d21aa21f2, Apr 20 2020, 13:19:08) [MSC v.1500 32 bit (Intel)]'
>>> np.typeDict['UInt64']
<type 'numpy.uint64'>
>>> np.__version__
1.16.6
>>>

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

note the 'I' in 'UInt64'

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The capitalized UInt form has already been deprecated - it's just the weird Uint form that isn't.

It looks to me like I added this type accidentally as part of giving [np.cint, np.int_, np.longlong] unique names. On numpy 1.11.0 (and python 2.7) the name isn't there at all:

>>> numpy.__version__
'1.11.0'
>>> sorted(k for k in numpy.typeDict.keys() if isinstance(k, str) and k[0].isupper() and len(k) > 2)
['Bool', 'Complex128', 'Complex32', 'Complex64', 'Datetime64', 'Float128', 'Float16', 'Float32', 'Float64', 'Int16', 'Int32', 'Int64', 'Int8', 'Object0', 'String0', 'Timedelta64', 'UInt16', 'UInt32', 'UInt64', 'UInt8', 'Unicode0', 'Void0']

So I think we can just remove this special case?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Oh, that makes it more recent. I think we could just remove it, but if we want to get rid of Datetime64 as well, I also don't see a real downside to just including it here...

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

for reference - the Uint64 name comes from uintp, due to these lines (97a2950#r39808190) in 1.16.

So on 32-bit platforms, there will be a Uint32 instead.

Let's either deprecate both or remove both.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

OK, I included Uint32, I think if the tests all pass things should be fine.

@seberg seberg added 07 - Deprecation 62 - Python API Changes or additions to the Python API. Mailing list should usually be notified. component: numpy.dtype labels Jun 9, 2020
This removes the types from usage for `np.dtype()`, since the
two are closely related, also finishes the deprecation of
`typeNA` and `sctypeNA` (thus removing them from the public API).

The deprecation for the numeric types was only shown for
`np.dtype("Complex64")`, etc. however, here they are also
removed from `np.typeDict` and `np.sctypeDict`.
@seberg seberg force-pushed the deprecate-numerictypes branch from b162875 to d4f9fb7 Compare June 9, 2020 21:18

from numpy.compat import unicode
from numpy._globals import VisibleDeprecationWarning
from numpy.core._string_helpers import english_lower, english_capitalize
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is english_capitalize used any more?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

No, I hardcoded the few exceptions...

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

My point is, can its definition be removed too?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Sorry for being unclear, yes it can be, there are no more usages in this file.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@eric-wieser one quick question to be sure. Uin32 was never defined on windows? Because otherwise I have to check for (and add it) here... (or remove both)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Uint32 was defined on 32-bit windows only.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Ahh, its a 32bit thing... In any case, will fix this up then.

Cool about gh-14882 seems there is consensus on moving forward with it, so I will definitely review and put that in soon!

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Sorry for being unclear, yes it can be, there are no more usages in this file.

Ping on this - we may as well remove it from _string_helpers if it has no other callers.

seberg and others added 2 commits June 10, 2020 08:11
seberg and others added 2 commits June 10, 2020 11:10
Co-authored-by: Eric Wieser <wieser.eric@gmail.com>
Co-authored-by: Eric Wieser <wieser.eric@gmail.com>
]
deprecated_types = ['Bytes0', 'Datetime64', 'Str0']
# Depending on intp size, either Uint32 or Uint64 is defined:
deprecated_types.append(f"U{np.dtype(np.intp).name}")
Copy link
Copy Markdown
Member

@eric-wieser eric-wieser Jun 10, 2020

Choose a reason for hiding this comment

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

Or

Suggested change
deprecated_types.append(f"U{np.dtype(np.intp).name}")
deprecated_types.append(np.dtype(np.uintp).name.title())

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Looks nicer, I am a bit uncertain about locales here since the other code used english_upper everywere?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Good point, safer to leave it as is.

@mattip mattip merged commit e0d27e3 into numpy:master Jun 10, 2020
@seberg seberg deleted the deprecate-numerictypes branch June 10, 2020 20:34
@TomAugspurger
Copy link
Copy Markdown

Thanks, will these changes end up in NumPy 1.19 or 1.20?

@seberg
Copy link
Copy Markdown
Member Author

seberg commented Jun 12, 2020

1.20, no intention to backport this, 1.19 may still get more critical fixes, this is not critical.

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

Labels

07 - Deprecation 62 - Python API Changes or additions to the Python API. Mailing list should usually be notified. component: numpy.dtype

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants