Skip to content

DEP: Deprecate passing shape=None to mean shape=()#15886

Merged
seberg merged 1 commit intonumpy:masterfrom
eric-wieser:deprecate-shape-None
May 31, 2020
Merged

DEP: Deprecate passing shape=None to mean shape=()#15886
seberg merged 1 commit intonumpy:masterfrom
eric-wieser:deprecate-shape-None

Conversation

@eric-wieser
Copy link
Copy Markdown
Member

This requires some minor tweaks in np.random because there the two have different meanings, with () meaning 0d array and None meaning scalar.

Made primarily to answer the question of how noisy this would be, from #15877 (comment).

@eric-wieser eric-wieser force-pushed the deprecate-shape-None branch 3 times, most recently from b92985a to 43490a2 Compare April 1, 2020 08:47
@eric-wieser eric-wieser marked this pull request as ready for review April 28, 2020 08:44
@eric-wieser
Copy link
Copy Markdown
Member Author

Probably needs testing against the scipy and pandas testsuites

@rgommers
Copy link
Copy Markdown
Member

Is there a concrete benefit besides it being a little cleaner?

@eric-wieser
Copy link
Copy Markdown
Member Author

eric-wieser commented Apr 28, 2020

Primarily that it will smoke out bugs downstream where shape=() was intended to mean "create a scalar", but has been implemented to mean "ignore this argument".

Having this warning locally was what discovered the bugs #15881, #15882, and #15885.

Copy link
Copy Markdown
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 assume that that nobody uses it in most cases. I looked through our own code base usage, and we seem fine. The main issue would be reduction-like functions not accepting None anymore.

The most unsure I was was whether descriptors should keep accepting None to mean no-subarray. A test for array creation functions might be good (they seem the most visible users), or test it directly with the new testing functions.

@eric-wieser
Copy link
Copy Markdown
Member Author

eric-wieser commented May 19, 2020

The most unsure I was was whether descriptors should keep accepting None to mean no-subarray.

As in, np.dtype((int, None)) and np.dtype([('a', int, None)])? As far as I can tell, these are already an error, albeit a very cryptic one.

A test for array creation functions might be good (they seem the most visible users), or test it directly with the new testing functions.

I'd forgotten I now have tests for the converters. A test for PyArray_IntpConverter seems very sensible.

@seberg
Copy link
Copy Markdown
Member

seberg commented May 19, 2020

Thanks, was just going to say that the tests seem still needed. The only usage in scipy is fine. If anyone needs it, I guess we could consider making the optional version public, but dunno...

@seberg
Copy link
Copy Markdown
Member

seberg commented May 19, 2020

@eric-wieser sorry for the followup, those commands (dtype([('a', '<i8')])) work for me on master? Or is this platform dependend somehow?

@eric-wieser
Copy link
Copy Markdown
Member Author

Huh, I think I was testing those on 1.17.0. Did we add support for None on purpose?

@seberg
Copy link
Copy Markdown
Member

seberg commented May 19, 2020

I don't recall adding it on purpose, was probably an accident... Anyway, it doesn't matter too much? Just deprecate it again now. I don't mind if None works, but it also doesn't seem very useful for me (and () is identical, which actually makes it confusing rather than helpful).

@seberg seberg closed this May 20, 2020
@seberg seberg reopened this May 20, 2020
@seberg seberg self-requested a review May 31, 2020 13:16
seberg pushed a commit that referenced this pull request May 31, 2020
@eric-wieser eric-wieser force-pushed the deprecate-shape-None branch from 4caa0ac to abb0c83 Compare May 31, 2020 16:20
This requires some minor tweaks in `np.random` because there the two have different meanings, with `()` meaning 0d array and `None` meaning scalar.
@eric-wieser eric-wieser force-pushed the deprecate-shape-None branch from abb0c83 to 5443f6d Compare May 31, 2020 16:24
@eric-wieser
Copy link
Copy Markdown
Member Author

The most unsure I was was whether descriptors should keep accepting None to mean no-subarray.

I still can't work out the case you're describing here. Can you give me the input/output pair that you're referring to

With this patch, the suggestions I mention above give ValueError: mismatch in size of old and new data-descriptor due to it attempting to construct a union type. I'm pretty sure master behaves the same way.

@seberg
Copy link
Copy Markdown
Member

seberg commented May 31, 2020

Eric:

In [1]: np.dtype((np.int64, None))                                                                                      
Out[1]: dtype('int64')

works for me, in master as well as 1.18

@eric-wieser
Copy link
Copy Markdown
Member Author

eric-wieser commented May 31, 2020

Got it, thanks. Works for me too in this branch with no warning. My issue was using int instead of int64.

@seberg
Copy link
Copy Markdown
Member

seberg commented May 31, 2020

Ah OK, I guess this is already special cased there, sorry for the noise.

@eric-wieser
Copy link
Copy Markdown
Member Author

I think I'm fine continuing to allow None in subdtype specs, to answer the original question.

@seberg
Copy link
Copy Markdown
Member

seberg commented May 31, 2020

Thanks for the test Eric, lets put this in.

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.

3 participants