Skip to content
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: Remove zero names from dtype aliases #24807

Merged
merged 2 commits into from Sep 29, 2023

Conversation

mtsokol
Copy link
Member

@mtsokol mtsokol commented Sep 26, 2023

Hi @rgommers @ngoldbaum @seberg,

This PR removes 7 names from the np.dtype(<str_alias>) search:

int0
uint0
void0
object0
str0
bytes0
bool8

as these have been deprecated in #22607 in 1.24 release. Their np.* types counterparts have been already removed from the main namespace.

For informational purposes I added if check in the C code that explicitly verifies if one of these names is provided to display a custom error message. I'm not sure how much overhead such additional check will have on np.dtype but it's only evaluated when the execution is going to fail anyway with data type *** not understood.

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.

Looks fine to me, not sure I am use dto black style in C code yet, but doesn't matter...

int0 going to intp (IIRC) is rather confusing, but I don't understand what the point of these ever were and this was already pretty much deprecated.

Anyway, happy with this, unless someone points out that this is used a lot downstream and I really doubt that here.

numpy/core/src/multiarray/descriptor.c Outdated Show resolved Hide resolved
@rgommers rgommers added this to the 2.0.0 release milestone Sep 29, 2023
Copy link
Member

@rgommers rgommers left a comment

Choose a reason for hiding this comment

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

LGTM, let's give this a go - thanks Mateusz & reviewers!

'bool', 'object',
'str', 'bytes', ('a', 'bytes_'),
('int0', 'intp'), ('uint0', 'uintp')]
'bool', 'object', 'str', 'bytes', ('a', 'bytes_')]
Copy link
Member

Choose a reason for hiding this comment

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

np.dtype('a') also seems ripe for removal, never seen it before and looks very odd. Does anyone know where that came from?

Copy link
Member Author

Choose a reason for hiding this comment

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

I've actually noticed np.dtype('a') but I couldn't figure out its origin and purpose 😄 - therefore I decided to keep it.

Copy link
Member

Choose a reason for hiding this comment

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

Looks like this goes back to 9ac3b0e. I have no idea why Travis added it and I don't know whether issues from the old trac issue tracker are still visible somewhere. Github search only finds 12 code usages of "np.dtype('a')". I say remove it.

Copy link
Member

Choose a reason for hiding this comment

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

Presumably it is meant to mean "ascii"... if almost nobody uses, it shouldn't matter, unlik "c" it dosn't eve have a special meaning.

@rgommers rgommers merged commit 241d676 into numpy:main Sep 29, 2023
50 checks passed
@mtsokol mtsokol deleted the remove-zero-name-aliases branch September 29, 2023 14:01
@EpicWink
Copy link

Am I to understand that these will be removed in NumPy v2.0? Or will it happen in a v1.x release?

@mtsokol
Copy link
Member Author

mtsokol commented Nov 16, 2023

Am I to understand that these will be removed in NumPy v2.0? Or will it happen in a v1.x release?

These changes are planned for NumPy 2.0 release. Here you can find all PRs with 2.0 changes label.

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.

None yet

6 participants