Skip to content

please consider adding index operator to numpy.h array_t #621

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

Closed
nbecker opened this issue Jan 26, 2017 · 5 comments
Closed

please consider adding index operator to numpy.h array_t #621

nbecker opened this issue Jan 26, 2017 · 5 comments

Comments

@nbecker
Copy link

nbecker commented Jan 26, 2017

With this change we can easily wrap old c++ code for python. Providing operator() makes it easy to transform, for example:
a[2][3] = 5;
double b = a[2][3];

into
a(2,3) = 5;
double b = a(2,3);

1 file changed, 8 insertions(+)
include/pybind11/numpy.h | 8 ++++++++

modified   include/pybind11/numpy.h
@@ -607,6 +607,14 @@ public:
         return static_cast<T*>(array::mutable_data(index...));
     }
 
+    template<typename... Ix> T& operator()(Ix... index) {
+      return *static_cast<T*>(array::mutable_data(index...));
+    }
+
+    template<typename... Ix> const T& operator()(Ix... index) const {
+      return *static_cast<const T*>(array::data(index...));
+    }
+
     // Reference to element at a given index
     template<typename... Ix> const T& at(Ix... index) const {
         if (sizeof...(index) != ndim())
@aldanor
Copy link
Member

aldanor commented Jan 26, 2017

There's array_t::at() and array_t::mutable_at(), why wouldn't that work for you? (or is it just the naming?)

@nbecker
Copy link
Author

nbecker commented Jan 26, 2017

It is just syntax - but syntax is important. It allows writing indexing in a way that looks familiar, and can be easily mechanically translated from other familiar forms.

@jagerman
Copy link
Member

It's consistent with Eigen's indexing as well. But that said, a(2,3) being the unsafe access while a.at(2,3) being the safe access (instead of the template policy in PR #617) seems pretty reasonable to me.

@jagerman
Copy link
Member

jagerman commented Mar 23, 2017

The one issue with an operator() on array_t<T> is the writable runtime flag annoyance, and I don't see a feasible way to deal with that since the reference returned by the operator is determined at compile time. If it returned a non-const reference, it would need a flags.writable check, but then wouldn't be usable for read-only code which would probably want to allow passing a non-writable array. if it returns a const reference, it would be annoying limitation that you can only read but not write the array.

The .unchecked()/.mutable_unchecked() partially addresses the notation, but only while also doing away with dimension checks, requiring a separate proxy object which is mildly inconvenient, and requiring the caller not to modify the referenced array's shape while the reference proxy exists.

Supporting this via the constness of *this (as suggested above) also won't work cleanly: it's entirely possible for a non-const array to be non-writeable (ensure(), for example, will reference an existing array if the input is compatible or make a copy if necessary (e.g. for requested contiguity or a required type conversion)).

So while I appreciate the syntactic benefit of having a a(1,2) work, the mismatch between numpy runtime writability and C++ compile-time constness doesn't lead to a nice solution.

@dean0x7d
Copy link
Member

dean0x7d commented Jun 7, 2017

The more convenient and faster operator() indexing has been implemented in #746 using unchecked references. The default access should remain with at() and mutable_at() for safety but it's pretty simple to access the unchecked() array and the convenient syntax. I think that at this point we have consensus that the split is necessary (as outlined in the PR).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants