reshape for numpy arrays #984

ncullen93 · 2017-08-06T07:35:17Z

implemented reshape, since resize is rarely useful and often fails.

Suggested changelog entry:

* Implemented ``reshape`` on arrays

rwgk · 2021-07-15T17:23:15Z

First impression: looks useful, filling in a blank, not doing anything substantially new or different, therefore unlikely to upset anything.

@Skylion007 if you want to modernize this one (CI green here), I could run all our sanitizers, if that's clean, merge.

I don't think this needs global testing. My testing turn-around could therefore be really quick.

…eshape

for more information, see https://pre-commit.ci

tests/test_numpy_array.py

…eshape

…to npyreshape

Skylion007 · 2021-07-28T18:32:08Z

@rwgk @henryiii , I fixed this PR.

tests/test_numpy_array.cpp

tests/test_numpy_array.py

rwgk · 2021-07-28T19:06:50Z

include/pybind11/numpy.h

@@ -790,6 +795,15 @@ class array : public buffer {
        if (isinstance<array>(new_array)) { *this = std::move(new_array); }
    }

+    array reshape(ShapeContainer new_shape) {


while we're at it, I'd also plumb through the order.
even if it's not actually used now (idk), it may be in future numpy versions.
easier to just do it than to explain why not.

@rwgk We would need to add support for it as a function arg, which we currently don't. That can be added in a followup PR.

That can be added in a followup PR.
OK

There is an NORDER Enum that it takes, but I am not sure how to expose that properly. We probably should leave it as is now.

sounds good, thanks for looking into it!

include/pybind11/numpy.h

rwgk · 2021-08-08T23:10:36Z

include/pybind11/numpy.h

@@ -790,6 +795,15 @@ class array : public buffer {
        if (isinstance<array>(new_array)) { *this = std::move(new_array); }
    }

+    array reshape(ShapeContainer new_shape) {


sounds good, thanks for looking into it!

rwgk · 2021-08-08T23:35:12Z

tests/test_numpy_array.py

+
+def test_reshape_tuple_empty(msg):
+    a = np.random.randn(10 * 20 * 30).astype("float64")
+    # FIXME: This should be a ValueError for maximum numpy compat


Yep, I just spent a couple minutes playing myself, we definitely need to make this compatible in this PR. Otherwise we'd be introducing a wrinkle of the really silly kind that can lead to surprisingly huge stumbles.

If you feel out of your comfort zone, I could help out working on it here. Please let me know.

I get the following error back:

E TypeError: Unable to convert function return value to a Python type! The signature was E (arg0: numpy.ndarray[numpy.float64], arg1: List[int]) -> numpy.ndarray

@rwgk Aha, figured it out. Behavior matches Numpy now.

henryiii · 2021-08-09T16:22:26Z

include/pybind11/numpy.h

@@ -203,6 +203,8 @@ struct npy_api {
    // Unused. Not removed because that affects ABI of the class.
    int (*PyArray_SetBaseObject_)(PyObject *, PyObject *);
    PyObject* (*PyArray_Resize_)(PyObject*, PyArray_Dims*, int, int);
+    PyObject* (*PyArray_Newshape_)(PyObject*, PyArray_Dims*, int);


This causes an ABI break?

@henryiii shrugs @rwgk thoughts?

I don't see any virtual here. If there is no vtable, how does adding or removing member functions change the ABI? I'm having doubts about the correctness of the comment in line 203. I could imagine maybe getting into trouble removing a function, but adding? If a newer extension knows about it, the machine code for it will be in that extension for sure. No?

rwgk · 2021-08-10T16:33:31Z

include/pybind11/numpy.h

+    array reshape(ShapeContainer new_shape) {
+        detail::npy_api::PyArray_Dims d
+            = {reinterpret_cast<Py_intptr_t *>(new_shape->data()), int(new_shape->size())};
+        // try to reshape, set ordering param to 0 cause it's not used anyway


I think it's best to remove this comment, and add a comment just above the array reshape(ShapeContainer new_shape) { line, e.g.

// Optional `order` parameter omitted here, to be added as needed.

rwgk · 2021-08-10T16:35:16Z

tests/test_numpy_array.cpp

+    sm.def("array_reshape1", [](py::array_t<double> a, size_t N) {
+        return a.reshape({N, N, N});
+    });


To keep the tests lean, I'd remove this function completely (and test_array_reshape with it). The exact same interface is covered by test_array_reshape. (I realize it takes away from PyPy coverage because of the xfail for test_array_reshape, but that's not enough reason in my mind to keep this one.)
If you feel differently, I'd at least change this to something like {N, N+5, N+7} to get a little bit more value.

rwgk · 2021-08-24T19:13:50Z

Hi @Skylion007, I streamlined the tests. Could you please take a look?
(PyPy doesn't need an xfail anymore.)

rwgk · 2021-08-26T15:12:20Z

Thanks Aaron for digging up this ancient PR, thanks Henry for the review. Sorry Nick this took forever, but we're getting better. Merging!

Skylion007 · 2021-08-26T15:12:42Z

This should be merged at same time as : #987

rwgk · 2021-08-26T15:15:21Z

This should be merged at same time as : #987

Yes, I need to run right now but back in an hour or two. If you get a chance to update it I'll take a look asap.

henryiii · 2021-09-16T21:26:32Z

These need to go into the docs, too.

ncullen93 and others added 7 commits August 6, 2017 03:29

reshape

6f8f47c

more tests

a08f012

Update numpy.h

36ed3a1

Update test_numpy_array.py

5fa0335

Update numpy.h

49a3b74

Update numpy.h

fdf05e6

Update test_numpy_array.cpp

afe7e75

bstaletic mentioned this pull request May 14, 2019

Unsorted pull requests YannickJadoul/pybind11-cleanup#8

Open

19 tasks

wjakob force-pushed the master branch from c4321d3 to cf36e3d Compare June 11, 2019 21:29

Skylion007 requested a review from rwgk July 15, 2021 17:07

Skylion007 and others added 2 commits July 15, 2021 14:13

Merge branch 'master' of https://github.com/pybind/pybind11 into npyr…

2c11c12

…eshape

[pre-commit.ci] auto fixes from pre-commit.com hooks

0fb238d

for more information, see https://pre-commit.ci

Skylion007 reviewed Jul 15, 2021

View reviewed changes

tests/test_numpy_array.py Outdated Show resolved Hide resolved

Skylion007 reviewed Jul 15, 2021

View reviewed changes

tests/test_numpy_array.py Outdated Show resolved Hide resolved

Skylion007 requested a review from henryiii July 15, 2021 18:38

Skylion007 added 6 commits July 28, 2021 13:50

Merge branch 'master' of https://github.com/pybind/pybind11 into npyr…

9366583

…eshape

Fix merge bug

d1ffc0f

Merge branch 'npyreshape' of https://github.com/ncullen93/pybind11 in…

612e01e

…to npyreshape

Make clang-tidy happy

4b31ca7

Add xfail for PyPy

2a486f5

Fix casting issue

56663b5

Skylion007 self-requested a review July 28, 2021 18:31

Skylion007 approved these changes Jul 28, 2021

View reviewed changes

rwgk reviewed Jul 28, 2021

View reviewed changes

henryiii reviewed Jul 28, 2021

View reviewed changes

include/pybind11/numpy.h Outdated Show resolved Hide resolved

Skylion007 added 3 commits August 8, 2021 13:57

Address reviews on additional tests

5e93212

Fix ordering

7685e21

Do a little more reordering

32c0cef

Skylion007 added 2 commits August 8, 2021 14:29

Fix typo

521a261

Try improving tests

b4d2094

Skylion007 requested review from rwgk and Skylion007 August 8, 2021 20:30

rwgk reviewed Aug 8, 2021

View reviewed changes

Fix error in reshape

cf143ae

Skylion007 requested review from henryiii and rwgk August 9, 2021 15:20

Add one more reshape test

fb92dc4

henryiii approved these changes Aug 9, 2021

View reviewed changes

henryiii reviewed Aug 9, 2021

View reviewed changes

rwgk approved these changes Aug 10, 2021

View reviewed changes

Ralf Grosse-Kunstleve added 2 commits August 24, 2021 10:48

Merge branch 'master' into npyreshape

127b400

streamlining new tests; removing a few stray msg

487404b

Skylion007 approved these changes Aug 26, 2021

View reviewed changes

rwgk merged commit 59ad1e7 into pybind:master Aug 26, 2021

github-actions bot added the needs changelog Possibly needs a changelog entry label Aug 26, 2021

henryiii removed the needs changelog Possibly needs a changelog entry label Sep 16, 2021

rwgk mentioned this pull request Feb 9, 2023

FWD pybind11 google/pybind11clif#984

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

reshape for numpy arrays #984

reshape for numpy arrays #984

ncullen93 commented Aug 6, 2017 •

edited by henryiii

Loading

rwgk commented Jul 15, 2021

Skylion007 commented Jul 28, 2021

rwgk Jul 28, 2021

Skylion007 Jul 28, 2021

rwgk Jul 28, 2021

Skylion007 Aug 8, 2021 •

edited

Loading

rwgk Aug 8, 2021

rwgk Aug 8, 2021

rwgk Aug 8, 2021

Skylion007 Aug 9, 2021

Skylion007 Aug 9, 2021

henryiii Aug 9, 2021

Skylion007 Aug 9, 2021

rwgk Aug 9, 2021

rwgk Aug 10, 2021

rwgk Aug 10, 2021

rwgk commented Aug 24, 2021

rwgk commented Aug 26, 2021

Skylion007 commented Aug 26, 2021

rwgk commented Aug 26, 2021

henryiii commented Sep 16, 2021

reshape for numpy arrays #984

reshape for numpy arrays #984

Conversation

ncullen93 commented Aug 6, 2017 • edited by henryiii Loading

Suggested changelog entry:

rwgk commented Jul 15, 2021

Skylion007 commented Jul 28, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Skylion007 Aug 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rwgk commented Aug 24, 2021

rwgk commented Aug 26, 2021

Skylion007 commented Aug 26, 2021

rwgk commented Aug 26, 2021

henryiii commented Sep 16, 2021

ncullen93 commented Aug 6, 2017 •

edited by henryiii

Loading

Skylion007 Aug 8, 2021 •

edited

Loading