8000 BUG: Move ndarray.dump to python and make it close the file it opens by eric-wieser · Pull Request #13684 · numpy/numpy · GitHub
[go: up one dir, main page]
Skip to content

BUG: Move ndarray.dump to python and make it close the file it opens #13684

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Jun 3, 2019
Merged

BUG: Move ndarray.dump to python and make it close the file it opens #13684

merged 2 commits into from
Jun 3, 2019

Conversation

eric-wieser
Copy link
Member

Spin off from #12915

@eric-wieser eric-wieser assigned mattip and unassigned mattip May 31, 2019
@eric-wieser eric-wieser requested a review from mattip May 31, 2019 16:12
mhvk
mhvk reviewed May 31, 2019
View reviewed changes
Copy link
Contributor
@mhvk mhvk left a comment

Choose a reason for hiding this comment

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

Yes, no need to reinvent python inside C, especially if we end up calling outside anyway.

One unneeded import.

@eric-wieser eric-wieser force-pushed the dump-to-python branch 2 times, most recently from cd0a628 to c702a30 Compare June 1, 2019 08:36
@eric-wieser
Copy link
Member Author

@mhvk: Import removed

@eric-wieser eric-wieser mentioned this pull request Jun 1, 2019
Merged
charris
charris reviewed Jun 1, 2019
View reviewed changes
mattip
mattip reviewed Jun 1, 2019
View reviewed changes
@mattip
Copy link
Member
mattip commented Jun 1, 2019

The end result is that ndarray.dump{,s} calls into c to PyArray_Dump{s,}, which calls out to python again numpy.core._methods.dump{,s}. It is too bad we cannot call the python implementation directly rather than going through C, since ndarray is a c-extension type

charris
charris reviewed Jun 1, 2019
View reviewed changes
@eric-wieser
Copy link
Member Author
eric-wieser commented Jun 1, 2019 •
edited
Loading

It is too bad we cannot call the python implementation directly rather than

We essentially can, but it results in bypassing PyArray_Dump{s} and leaving it with no tests (updated to do this anyway)

However, given we deprecated np.loads, perhaps we should just deprecate ndarray.dumps and not worry about this.

@eric-wieser
BUG: Move ndarray.dump to python and make it close the file it opens
87eeb8d 
As a side-effect, this makes it support kwargs.
@mattip
Copy link
Member
mattip commented Jun 2, 2019

@charris
Copy link
Member
charris commented Jun 2, 2019

With the fixed return this looks good to me. Would be nice to have a test for np.dumps, something like assert_equal(a, pickle.loads(a.dumps())), or an issue to deprecate it.

We should probably deprecate it at some point. ISTR that that has been mentioned before.

@charris
Copy link
Member
charris commented Jun 2, 2019

A test would be nice as long as this is still out there.

@eric-wieser
Copy link
Member Author

Thanks for adding that @mattip

@charris charris merged commit 52d173d into numpy:master Jun 3, 2019
@charris
Copy link
Member
charris commented Jun 3, 2019

Thanks @eric-wieser @mattip.

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

@charris charris charris left review comments

@mattip mattip mattip left review comments

@mhvk mhvk mhvk left review comments

Assignees
No one assigned
Labels
00 - Bug component: numpy._core
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@eric-wieser @mattip @charris @mhvk
0