eigen: Disable dtype=object arrays from being referenced #17
Conversation
EricCousineau-TRI
force-pushed
the
feature/disable_dtype_object_ref
branch
3 times, most recently
from
6e6e595 to
f8d6b82
Compare
| case return_value_policy::move: | ||
| case return_value_policy::copy: | ||
| case return_value_policy::automatic_reference: | ||
| return eigen_array_cast<props>(*src); |
There was a problem hiding this comment.
Maybe we can alphabetize, or use the same ordering as above, or this one?
http://pybind11.readthedocs.io/en/stable/advanced/functions.html
There was a problem hiding this comment.
OK This actually kind of uses the above ordering, but split between success and failure now.
If you look at the subset of cases split between success and failure, you'll see they're in the same order.
include/pybind11/eigen.h
Outdated
| if (is_pyobject_<Scalar>()) { | ||
| is_pyobject = true; | ||
| constexpr bool is_pyobject = is_pyobject_dtype<Scalar>::value; | ||
| // TODO(eric.cousineau): Re-enable this once this does not aversely |
There was a problem hiding this comment.
I don't understand when we will be enabling the assert?
There was a problem hiding this comment.
It would be re-enabled once RobotLocomotion/drake#8452 either merges (in which case no code should run across this) or fails (in which case we want to quickly squash these errors).
As of RobotLocomotion/drake#8665, I've only taken basic steps to support multiple scalar types, some of which compiles this code. I want a run-time error now, so that if anybody uses them right now, they get a quick error, but everything still compiles for the high-level API checks on CI.
Once one of the above happens, then it make sense to elevate to compile-time to more quickly squash its usage.
Clarified the comment too.
tests/test_eigen.cpp
Outdated
| @@ -108,7 +108,7 @@ TEST_SUBMODULE(eigen, m) { | |||
| m.def("double_threec", [](py::EigenDRef<Eigen::Vector3f> x) { x *= 2; }); | |||
| m.def("double_adscalarc", [](py::EigenDRef<VectorXADScalar> x) { x *= 2; }); | |||
| m.def("double_threer", [](py::EigenDRef<Eigen::RowVector3f> x) { x *= 2; }); | |||
| m.def("double_adscalarr", [](py::EigenDRef<VectorXADScalarR> x) { x *= 2; }); | |||
| // m.def("double_adscalarr", [](py::EigenDRef<VectorXADScalarR> x) { x *= 2; }); | |||
There was a problem hiding this comment.
Done. Fixed up the tests to have more coverage.
include/pybind11/eigen.h
Outdated
| @@ -233,17 +230,22 @@ template <typename props> handle eigen_array_cast(typename props::Type const &sr | |||
| src.data(), base); | |||
| } | |||
| else { | |||
| if (base) { | |||
| // Should be disabled by `cast_impl`. | |||
| throw cast_error("should not have reached here (dtype=object)"); | |||
There was a problem hiding this comment.
Do we need to test this? We are testing just dtype=object in our test which is present in this string?
There was a problem hiding this comment.
Done. Tried covering this more comprehensively; at this point, I want it to fail mainly; at later points, I would like it to fail at more defined locations. (Added a TODO.)
EricCousineau-TRI
force-pushed
the
feature/disable_dtype_object_ref
branch
from
f8d6b82 to
891c253
Compare
|
Reviewed 1 of 4 files at r1, 3 of 3 files at r2. Comments from Reviewable |
In preparation for scalar type conversion with
dtype=object, we should ensure that copies should be made when able, but we should error out as early as possible if a user attempts tries to reference existing memory when usingdtype=object.This can help point out areas that can be helped by a custom user-defined dtype, or something that should be reworked in it's generic form.
This change is