-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Add ability to create object matrices #1152
base: master
Are you sure you want to change the base?
Conversation
d9b8129
to
2c67e2a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added some lightweight comments down below (just as a commentator, nothing authoritative for pybind11
).
include/pybind11/eigen.h
Outdated
@@ -10,6 +10,7 @@ | |||
#pragma once | |||
|
|||
#include "numpy.h" | |||
#include <numpy/ndarraytypes.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW minor formatting nit This uses a different include style <>
than above ""
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
include/pybind11/eigen.h
Outdated
a = array({ src.rows(), src.cols() }, { elem_size * src.rowStride(), elem_size * src.colStride() }, | ||
src.data(), base); | ||
using Scalar = typename props::Type::Scalar; | ||
if (props::vector) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW minor formatting nit Could you (a) use is_pyobject
and (b) "transpose" and reverse this nested if
statement, just to keep the original code close together?
e.g.
if (!is_pyobject) {
if (props::vector) { ... }
else { ... }
} else {
... // new code
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
include/pybind11/eigen.h
Outdated
|
||
int result = detail::npy_api::get().PyArray_CopyInto_(ref.ptr(), buf.ptr()); | ||
if (npy_format_descriptor<Scalar>::value == npy_api::NPY_OBJECT_) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW For consistency, could you place this value into is_py_object
? And same as above, could this be transposed / reversed to keep the original code more visible?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
@@ -453,15 +543,55 @@ struct type_caster< | |||
// We need to copy: If we need a mutable reference, or we're not supposed to convert |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW Can this if
statement be joined to the one above via an else
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if this can be done, need_copy
is being changed inside if(!need_copy)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, missed that!
Can I ask if there's a possibility of incorporating this into the unittests that are present (in Also, naive question, but even if not using e.g. class Test {
public:
Test() { x_ << Variable("a"), Variable("b"); }
VectorX<Variable>& mutable_x() { return x_; } // bind with return_value_policy::reference
private:
VectorX<Variable> x_(2);
} and in Python: >>> t = Test()
>>> x = Test.mutable_x()
>>> x[1] = Variable('z')
>>> print(x)
[a, z]
>>> print(Test.mutable_x())
# Will this print "[a, b]" or "[a, z]"? EDIT: Per @jagerman's earlier comment:
It seems like this could be possible if there is some sort of "hot swap" with the incoming Python (That being said, I wouldn't expect this kind of feature to come in at this point. Possibly just for future consideration.) EDIT 2: May move this to the original issue conversation at some point. |
2c67e2a
to
ebbcce8
Compare
I apologize, again, for the lack of tests, I'm working on them. In the first pass I was wondering if there are any egregious mistakes in the approach, and the replacements for
I'm not sure if I'm answering your question. But my understanding is that Ref which when supported for PODTypes didn't allow non-writable matrices because it was transparently making changes numpy.ndarry. Since I am making an explicit copy, I let go of this restriction and don't return false even if the matrix is not writable if the array is of non-POD type. Quoting from their docs:
So, if I'm passing a numpy matrix of When your code is run with However, the behviour with doubles which I outlined wasn't there before, probably because we were not passing |
ebbcce8
to
3f45ee7
Compare
e1657ea
to
c62f5c8
Compare
c62f5c8
to
ffcf754
Compare
For a follow-up to this PR, there may be a way to generally avoid the need for object copies / using https://docs.scipy.org/doc/numpy-1.13.0/user/c-info.beyond-basics.html#user-defined-data-types |
FTR, my above comments about avoiding copies are out of scope for this PR, and should be excluded from consideration for now. We have some modifications in our fork relevant to this PR; I would be happy to help bring them in. |
Hello @EricCousineau-TRI @wjakob, @jagerman I work on the new flight dynamics software infrastructure for the European Space Agency. This software will be used to fly missions such as SOLO, BepiColombo, JUICE and more (see https://www.esa.int/ESA/Our_Missions). One major selling point is the ease-of-use using the python interface. We plan to make the software open source and available to European industry and academia. I am very interested in this PR, as it would add the possibility to handle automatic differentiation much easier on the C++/python interface level with Eigen/numpy. Due to strict third-party policy, pybind11 is already a library I have to push to management (so forks are hard to sell). @wjakob @jagerman Do you have a schedule on the next release and/or a discussion on which PR-s to include? Thank you for your time. |
Hi there, I would like to refresh the ping on this PR. In the meantime I have used https://github.com/RobotLocomotion/pybind11 to implement my use-case of object matrices, specifically using custom autodiff scalar types together with Eigen/numpy interface. In the end, I would prefer using the primary pybind11 repository. This PR might need to be updated with the latest changes/optimisations/bugfixes from the fork @EricCousineau-TRI @m-chaturvedi are you still maintaining that? I see @henryiii has been doing some nice work on the repo, do you see a chance this feature could be added? Let me know @wjakob @jagerman if you have any thoughts. Thank you in advance! |
I'd also be interested in creating and processing Numpy arrays of It is not super clear to me if this PR would (partially) enable that, though, as it also involves Eigen stuff (which I'm not using). I've also looked at the https://github.com/RobotLocomotion/pybind11 fork and the various issues linked to #2259 but it was hard for me (due to my lack of experience) to find out which information or code addition is relevant for Numpy object arrays. More specifically, I've tried reusing https://github.com/RobotLocomotion/pybind11/blob/drake/include/pybind11/numpy.h#L1282-L1298 and https://github.com/eacousineau/repro/blob/43407e3/python/pybind11/custom_tests/pybind11_numpy_scalar.h but didn't succeed in creating or processing Numpy objects array from C++. Is there something I'm missing? I've also noticed the ongoing efforts to allow the creation of custom ufuncs (e.g., in #1325), which is something I'd really want to be able to do for Numpy object arrays. Is there a workaround for this implemented in RobotLocomotion's pybind11 fork or in @EricCousineau-TRI, if you have any pointer to relevant code parts and/or examples that would be super helpful! |
Please excuse the lack of tests.This PR is towards issue #647 that @mwoehlke-kitware had created. I'm trying to get feedback of the developers on the PR. Please let me know of the mistakes I've made and the changes you'd like.Basically I'm explicit copying the
Eigen::Matrix
to thearray
type and vice-versa incast
andload
respectively. For the load forEigen::Ref
as well, I make an explicit copy into aEigen::Ref
I created. I register the type usingPYBIND11_NUMPY_OBJECT_DTYPE
.