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

DEP: Actually deprecate np.dtype('O8') #15822

Conversation

eric-wieser
Copy link
Member

The code before tried to:

  • Deprecate np.dtype('O8')
  • Silently allow loading pickles containing 'O8'

But actually due to a logic bug:

  • Silently allowed np.dtype('O8')
  • Deprecated loading pickles containing 'O8'

Since we already deprecated the pickles for 8 years by accident, we may as well just deprecate both uses now.

@eric-wieser eric-wieser force-pushed the fix-evil_global_disable_warn_O4O8_flag branch 2 times, most recently from a63cce2 to 59668c9 Compare March 24, 2020 13:47
seberg
seberg previously approved these changes Mar 24, 2020
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.

LGTM, will probably just commit the suggestion and merge myself later. Fun typo, just shows that even simple code is better with tests :).

numpy/core/src/multiarray/conversion_utils.c Outdated Show resolved Hide resolved
@seberg
Copy link
Member

seberg commented Mar 24, 2020

Hmm, a bit more fallout than I thought, there is an actual tests unpickling an O8 dtype that fails currently.

@eric-wieser
Copy link
Member Author

Hmm, a bit more fallout than I thought, there is an actual tests unpickling an O8 dtype that fails currently.

I was under the impression that when unpickling the warning was already emitted. Am I mistaken?

@seberg
Copy link
Member

seberg commented Mar 24, 2020

I am as confused as you are, but round tripping tests for npz are failing for example:
https://dev.azure.com/numpy/numpy/_build/results?buildId=8798&view=logs&j=25bb66cf-4a16-533e-490c-8da4b5a3ec04&t=97446ee6-ebce-5662-24e5-f82eed312005&l=802

Probably this is not array but the single dtype pickle. However, the roundtripping should certainly work, so I guess the state setting may be off, since it should not be storing O8 to begin with?

@eric-wieser
Copy link
Member Author

Here's the issue:

>>> np.dtype(object).__reduce__()
(numpy.dtype, ('O8', False, True), (3, '|', None, None, None, -1, -1, 63))

* that it is in an unpickle context instead of a normal context without
* evil global state like we create here.
*/
NPY_NO_EXPORT int evil_global_disable_warn_O4O8_flag = 0;
Copy link
Member

Choose a reason for hiding this comment

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

can we also get rid of extern declaration in conversion_utils.h

eric-wieser and others added 3 commits May 4, 2020 12:10
The code before tried to:
* Deprecate `np.dtype('O8')`
* Silently allow loading pickles containing `'O8'`

But actually due to a logic bug:
* Silently allowed `np.dtype('O8')`
* Deprecated loading pickles containing `'O8'`

Since we already deprecated the pickles for 8 years by accident, we may as well just deprecate both uses now.
@seberg seberg force-pushed the fix-evil_global_disable_warn_O4O8_flag branch from 64e38d0 to 429513f Compare May 4, 2020 17:53
@mattip mattip dismissed seberg’s stale review May 6, 2020 06:13

There are still failing tests

@mattip
Copy link
Member

mattip commented May 6, 2020

There are still failing tests

@charris
Copy link
Member

charris commented Dec 16, 2020

@eric-wieser Ping. Do you want to keep this open? You have a bunch of PRs, it would be helpful if you looked through them and decided which you would like to keep open.

Base automatically changed from master to main March 4, 2021 02:04
@charris charris added the 52 - Inactive Pending author response label Apr 6, 2022
@charris charris closed this Apr 6, 2022
@seberg seberg added the 64 - Good Idea Inactive PR with a good start or idea. Consider studying it if you are working on a related issue. label Apr 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
00 - Bug 07 - Deprecation 52 - Inactive Pending author response 64 - Good Idea Inactive PR with a good start or idea. Consider studying it if you are working on a related issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants