Fix arrays with zero-size dimensions #58
Conversation
|
We can't merge patches here unless we also open a Drake PR that refers to this PR, to prove that Drake CI passes (not just RobotLocomotion/pybind11 CI). Please open the Drake PR and mention it here. |
mwoehlke-kitware
force-pushed
the
drake-fix-zero-size-arrays
branch
from
2e20d0d to
3eea2a2
Compare
Update to newer pybind11 to address a change in numpy 1.23 that caused some arrays to fail to be converted to Eigen matrices. See RobotLocomotion/pybind11#58.
|
Drake CI succeeded for RobotLocomotion/drake#17475. Particularly:
...which should be using numpy 1.23 and thus would be failing without this change. Meanwhile, upstream merged a version with (non-functional) changes. I'd rather not wait on all the CI to re-run; is it going to be a problem for whoever ends up merging upstream to remember to just take their version, or should I tweak this to more closely match the versions that's now upstream? |
|
Definitely our patch should be as similar to upstream as possible. If the timeline means foregoing another spin of Drake CI, so be it. |
When converting an array to an Eigen matrix, ignore the strides if any dimension size is 0. If the array is empty, the strides aren't relevant, and especially numpy ≥ 1.23 explicitly sets the strides to 0 in this case. (See numpy commit dd5ab7b11520.) Update tests to verify that this works, and continues to work. (This test likely would have caught the behavior change with numpy 1.23.) This expands on the tests added by pybind#38, but also reverts the logic change from that PR, as it is no longer needed, and brings the code closer in line with upstream, since pybind#38 was never pushed upstream.
mwoehlke-kitware
force-pushed
the
drake-fix-zero-size-arrays
branch
from
3eea2a2 to
8051585
Compare
Okay, done. It should be easy enough to verify in Reviewable that there is no functional change, just aesthetic changes. Once this lands, I'll update the Drake PR to point to the post-merge SHA. |
|
...or it would have been if Reviewable had kept the previous version. Since it looks like it didn't, here's that diff: - if (rows == 0 || cols == 0)
- return !negativestrides;
- return
- !negativestrides &&
- (props::inner_stride == Eigen::Dynamic || props::inner_stride == stride.inner() ||
- (EigenRowMajor ? cols : rows) == 1) &&
- (props::outer_stride == Eigen::Dynamic || props::outer_stride == stride.outer() ||
- (EigenRowMajor ? rows : cols) == 1);
+ if (negativestrides) {
+ return false;
+ }
+ if (rows == 0 || cols == 0) {
+ return true;
+ }
+ return (props::inner_stride == Eigen::Dynamic || props::inner_stride == stride.inner()
+ || (EigenRowMajor ? cols : rows) == 1)
+ && (props::outer_stride == Eigen::Dynamic || props::outer_stride == stride.outer()
+ || (EigenRowMajor ? rows : cols) == 1);Note also that everything starting with |
Update to newer pybind11 to address a change in numpy 1.23 that caused some arrays to fail to be converted to Eigen matrices. See RobotLocomotion/pybind11#58.
When converting an array to an Eigen matrix, ignore the strides if any dimension size is 0. If the array is empty, the strides aren't relevant, and especially numpy ≥ 1.23 explicitly sets the strides to 0 in this case. (See numpy/numpy@dd5ab7b11520.)
Update tests to verify that this works, and continues to work. (The updated test would have caught the behavior change with numpy 1.23.) This expands on the tests added by #38, but also reverts the logic change from that PR, as it is no longer needed, and brings the code closer in line with upstream, since #38 was never pushed upstream.
+@EricCousineau-TRI for review, please.
See also pybind#4038.
This change is