array: implement array resize #795

uentity · 2017-04-11T20:10:54Z

Resize won't work in every conditions, but at least it should work

if new array was created from scratch in C++ and resized before being exported to Python
if array passed from Python, owns it's data and doesn't have dependant arrays and refcheck is 0.

jagerman · 2017-04-11T20:24:04Z

include/pybind11/numpy.h

@@ -158,6 +163,7 @@ struct npy_api {
                                             Py_ssize_t *, PyObject **, PyObject *);
    PyObject *(*PyArray_Squeeze_)(PyObject *);
    int (*PyArray_SetBaseObject_)(PyObject *, PyObject *);
+    PyObject* (*PyArray_Resize_)(pybind11::detail::PyArray_Proxy*, PyArray_Dims*, int, int);


I think the first argument should just be a PyObject *, to be consistent with the other function pointers (e.g. PyArray_Squeeze_).

jagerman · 2017-04-11T20:25:37Z

include/pybind11/numpy.h

@@ -646,6 +654,26 @@ class array : public buffer {
        return reinterpret_steal<array>(api.PyArray_Squeeze_(m_ptr));
    }

+    /// Resize array to given shape
+    /// If refcheck is true, then resize will fail if > 1 references exists to this array
+    void resize(const std::vector<size_t> &new_shape, bool refcheck = false) {


I strongly feel that refcheck should default to true. Bypassing a reference check can be pretty dangerous, and should never be the default.

Also the comment here is a little bit wrong: resize fails with > 1 reference only if changing the total size. If the size doesn't change, a resize should be fine. (It's probably also worth changing the test to explicitly test this; at = a.transpose() is a simple way to add a reference to a).

+1 for refcheck=true by default.

Also note that in the Python API, refcheck defaults to True.

jagerman · 2017-04-11T20:55:09Z

include/pybind11/numpy.h

+        // try to resize, set order to -1 cause it's not used anyway
+        new_array = detail::npy_api::get().PyArray_Resize_(
+            detail::array_proxy(m_ptr), &d, int(refcheck), -1
+        );


Rather than use raw pointers, better to use pybind::object to make sure you get proper reference handling. This should work:

object new_array = reinterpret_steal<object>(detail::npy_api::get().PyArray_Resize_( detail::array_proxy(m_ptr), &d, int(refcheck), -1 )); if (!new_array) throw error_already_set(); if (isinstance<array>(new_array)) { *this = std::move(new_array); }

The main difference, aside from simplifying the code a bit, is that when the return is PyNone, you still properly decref it at the end of the function when the object is destroyed (unless you move it away first). Right now it leaks a reference--which probably isn't that big a deal with None, but still.

Somewhat strange to me about PyArray_Resize() is that, looking at the source code, I can't see how it ever returns anything other than None (aside from NULL returned on error).

jagerman · 2017-04-11T21:03:06Z

tests/test_numpy_array.py

@@ -377,3 +377,18 @@ def test_array_unchecked_dyn_dims(msg):

    assert proxy_auxiliaries2_dyn(z1) == [11, 11, True, 2, 8, 2, 2, 4, 32]
    assert proxy_auxiliaries2_dyn(z1) == array_auxiliaries2(z1)
+
+def test_array_resize(msg):


You should also test some failure conditions here, i.e. that you get the expected exception and exception message if attempting to resize a referenced array.

jagerman · 2017-04-11T21:05:50Z

tests/test_numpy_array.cpp

@@ -264,4 +264,24 @@ test_initializer numpy_array([](py::module &m) {
    sm.def("array_auxiliaries2", [](py::array_t<double> a) {
        return auxiliaries(a, a);
    });
+
+    // resize array to 3D without changing size


3D -> 2D

jagerman · 2017-04-11T21:08:51Z

tests/test_numpy_array.cpp

+    // resize array to 3D without changing size
+    // should probably succeed
+    sm.def("array_reshape2", [](py::array_t<double> a) {
+        const size_t dim_sz(std::sqrt(a.size()));


A self-commenting input validation test:

if (dim_sz * dim_sz != a.size()) throw std::domain_error("array_reshape2: input array total size is not a squared integer");

jagerman · 2017-04-11T23:20:44Z

A few things to fix, but nothing major: this looks pretty good to me.

dean0x7d · 2017-04-12T00:32:02Z

There is also the possibility of simplifying the implementation so it doesn't need the Numpy C API:

void resize(const std::vector<size_t> &new_shape, bool refcheck = true) {
    attr("resize")(new_shape, refcheck);
}

Which would be a bit slower because of the dynamic method lookup, but at the same time I don't think resize is going into any hot loops anyway. (I don't mind the C API either, I'm just mentioning this as a possibility.)

jagerman · 2017-04-12T15:33:03Z

include/pybind11/numpy.h

-    void resize(const std::vector<size_t> &new_shape, bool refcheck = false) {
+    /// If refcheck is true and more that one reference exist to this array
+    /// then resize will succeed only if it makes a reshape, i.e. original size doesn't change
+    void resize(const std::vector<size_t> &new_shape, bool refcheck = true) {


I'm not a huge fan of taking concrete containers (std::vector<size_t>) like this--it's restrictive and something of a nuisance if you happen to have your shape in some other container. Just to be clear: this isn't your fault at all; what you wrote here is perfectly consistent with the current API. But I'm aiming to solve it with PR #788, which would allow you to change the signature to void resize(ShapeContainer new_shape, bool refcheck = true), where the ShapeContainer is implicitly convertible from any sort of container, copy the contents into a std::vector<Py_intptr_t>. That would also allow you to drop both the reinterpret_cast and const_cast from:
reinterpret_cast<Py_intptr_t*>(const_cast<size_t*>(new_shape.data())), int(new_shape.size())
replacing it with just:
new_shape->data(), int(new_shape.size())

I've merged #788, so you should be able to switch the input to a ShapeContainer new_shape now. (Unfortunately I also caused some conflicts--both had tests added in the same place--but I added a commit to hopefully resolve them).

aldanor · 2017-04-13T14:10:42Z

(Minor: it's best to not have merge commits from master in feature branches, ideally this PR would get rebased or squashed)

jagerman · 2017-04-13T14:19:51Z

Yeah, it'll get squashed and rebased before being merged.

uentity · 2017-04-13T16:47:48Z

include/pybind11/numpy.h

@@ -129,6 +129,11 @@ struct npy_api {
        NPY_STRING_, NPY_UNICODE_, NPY_VOID_
    };

+    typedef struct {
+        const Py_intptr_t *ptr;


I think it's safe to make ptr const here instead of having const_cast<Py_intptr_t*>(new_shape->data()) later in resize()

Just change the const ShapeContainer &new_shape to ShapeContainer new_shape; it's essentially always going to be passed by value (via implicit conversion), so the const reference doesn't gain anything.

Edit: the point being to keep the definition identical to the one numpy defines.

uentity · 2017-04-13T16:48:41Z

tests/test_numpy_array.cpp

@@ -264,7 +264,28 @@ test_initializer numpy_array([](py::module &m) {
        return auxiliaries(a, a);
    });

-    // Issue #785: Uninformative "Unknown internal error" exception when constructing array from empty object:
+      // Issue #785: Uninformative "Unknown internal error" exception when constructing array from empty object:


jagerman · 2017-04-13T17:28:00Z

You can ignore the MSVC failure; that's something broken in appveyor (#792).

uentity · 2017-04-14T17:21:00Z

Travis also didn't complete for some reason...

jagerman · 2017-04-14T21:06:53Z

I just restarted the failed job.

jagerman · 2017-04-19T02:09:57Z

Can you rebase this against current master? I have a feeling the travis-ci commit I merged today is going to cause some failures in the PyPy test here from some things numpy doesn't support when under PyPy (and so need to be skipped for PyPy).

uentity · 2017-04-19T19:27:26Z

Rebased.
I failed to build tests with latest PyPy (5.7.1) as well as with bundled with Fedora 25 (5.4.0), so didn't check if tests succeeds on it.

jagerman · 2017-04-19T20:10:42Z

Don't worry about 5.4.0; we don't support anything before 5.7 as it had several fixes we reported to get it working with pybind. The one test failure was exactly what I expected to fail; just skip that test under PyPy: move the failing assertions (basically anything with refcheck=True) into a separate test and mark that test with @pytest.unsupported_on_pypy

jagerman · 2017-04-19T20:12:12Z

I failed to build tests

Wait, failed to build? That part should work (with 5.7 at least).

uentity · 2017-04-20T04:39:16Z

Year, I see what you have done with disabling tests on PyPy in prev commits. I just wanted to check it myself before pushing.

But as I can see now, you build tests on PyPy 5.7 with Python 2.7 and I tried to build against PyPy 5.7 with Python 3.5. That combination gives compile error:

class_support.h:169:13: error: «struct _heaptypeobject» has no member named «ht_qualname»; did you mean «ht_name»?
  heap_type->ht_qualname = name_obj.inc_ref().ptr();
             ^~~~~~~~~~~

wjakob · 2017-04-20T07:49:02Z

Aha, looks like some Python 3.x compatibility issues in the Python 3 version of PyPy. There is no need to worry about it here.

jagerman · 2017-04-20T14:13:32Z

Oh, right, I forgot about the PyPy3 version. We currently only support PyPy2 5.7.

uentity · 2017-04-21T09:24:44Z

Yes, I noticed that support is only for PyPy2 5.7.
Are there any stoppers preventing this from being merged upstream?

dean0x7d · 2017-04-29T12:30:31Z

This looks good to me. There's a merge conflict right now. Just resolve that and I think it's good to merge.

jagerman · 2017-04-29T15:50:52Z

Can you do a rebase against master rather than a merge? It makes it easier to commit without getting merge commits in the log.

uentity · 2017-04-29T18:27:05Z

Sorry, I happened to use this slick github "resolve merge conflicts" button.

jagerman · 2017-04-29T19:20:06Z

Merged! Thanks!

jagerman reviewed Apr 11, 2017

View reviewed changes

jagerman mentioned this pull request Apr 12, 2017

Generic containers/iterators for shape/strides #788

Merged

jagerman reviewed Apr 12, 2017

View reviewed changes

uentity force-pushed the array-resize branch from d8bc720 to 0e8ef31 Compare April 13, 2017 16:44

uentity commented Apr 13, 2017

View reviewed changes

uentity force-pushed the array-resize branch from 0e8ef31 to b654aff Compare April 13, 2017 17:19

jagerman mentioned this pull request Apr 14, 2017

Travis: switch to trusty, pip and libc++ caching, pypy numpy #798

Merged

uentity force-pushed the array-resize branch from b654aff to 3773cf6 Compare April 19, 2017 19:23

uentity force-pushed the array-resize branch from 3773cf6 to ac83c33 Compare April 19, 2017 19:55

uentity force-pushed the array-resize branch from ac83c33 to 92a88a8 Compare April 20, 2017 04:17

array: implement array resize

830d528

uentity force-pushed the array-resize branch from b264345 to 830d528 Compare April 29, 2017 18:25

jagerman merged commit 083a021 into pybind:master Apr 29, 2017

dean0x7d modified the milestone: v2.2 Aug 13, 2017

rwgk mentioned this pull request Feb 9, 2023

FWD pybind11 google/pybind11clif#795

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

array: implement array resize #795

array: implement array resize #795

uentity commented Apr 11, 2017

jagerman Apr 11, 2017

jagerman Apr 11, 2017

jagerman Apr 11, 2017

dean0x7d Apr 12, 2017

jagerman Apr 12, 2017

jagerman Apr 11, 2017

jagerman Apr 11, 2017

jagerman Apr 11, 2017

jagerman Apr 11, 2017

jagerman commented Apr 11, 2017

dean0x7d commented Apr 12, 2017

jagerman Apr 12, 2017

jagerman Apr 13, 2017

aldanor commented Apr 13, 2017

jagerman commented Apr 13, 2017

uentity Apr 13, 2017 •

edited

Loading

jagerman Apr 13, 2017 •

edited

Loading

uentity Apr 13, 2017

uentity Apr 13, 2017

jagerman commented Apr 13, 2017

uentity commented Apr 14, 2017

jagerman commented Apr 14, 2017

jagerman commented Apr 19, 2017

uentity commented Apr 19, 2017

jagerman commented Apr 19, 2017

jagerman commented Apr 19, 2017

uentity commented Apr 20, 2017

wjakob commented Apr 20, 2017

jagerman commented Apr 20, 2017

uentity commented Apr 21, 2017

dean0x7d commented Apr 29, 2017

jagerman commented Apr 29, 2017

uentity commented Apr 29, 2017

jagerman commented Apr 29, 2017

array: implement array resize #795

array: implement array resize #795

Conversation

uentity commented Apr 11, 2017

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

jagerman commented Apr 11, 2017

dean0x7d commented Apr 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aldanor commented Apr 13, 2017

jagerman commented Apr 13, 2017

uentity Apr 13, 2017 • edited Loading

Choose a reason for hiding this comment

jagerman Apr 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jagerman commented Apr 13, 2017

uentity commented Apr 14, 2017

jagerman commented Apr 14, 2017

jagerman commented Apr 19, 2017

uentity commented Apr 19, 2017

jagerman commented Apr 19, 2017

jagerman commented Apr 19, 2017

uentity commented Apr 20, 2017

wjakob commented Apr 20, 2017

jagerman commented Apr 20, 2017

uentity commented Apr 21, 2017

dean0x7d commented Apr 29, 2017

jagerman commented Apr 29, 2017

uentity commented Apr 29, 2017

jagerman commented Apr 29, 2017

uentity Apr 13, 2017 •

edited

Loading

jagerman Apr 13, 2017 •

edited

Loading