Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix arrays with zero-size dimensions #4038

Merged
merged 1 commit into from
Jun 29, 2022
Merged

Fix arrays with zero-size dimensions #4038

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 10 additions & 4 deletions include/pybind11/eigen.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,10 +111,16 @@ struct EigenConformable {
bool stride_compatible() const {
// To have compatible strides, we need (on both dimensions) one of fully dynamic strides,
// matching strides, or a dimension size of 1 (in which case the stride value is
// irrelevant)
return !negativestrides
&& (props::inner_stride == Eigen::Dynamic || props::inner_stride == stride.inner()
|| (EigenRowMajor ? cols : rows) == 1)
// irrelevant). Alternatively, if any dimension size is 0, the strides are not relevant
// (and numpy ≥ 1.23 sets the strides to 0 in that case, so we need to check explicitly).
if (negativestrides) {
return false;
}
if (rows == 0 || cols == 0) {
return true;
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is more readable (avoids repeating !negativestrides):

-        // irrelevant)
-        return !negativestrides
-               && (props::inner_stride == Eigen::Dynamic || props::inner_stride == stride.inner()
-                   || (EigenRowMajor ? cols : rows) == 1)
+        // irrelevant). Alternatively, if any dimension size is 0, the strides are not relevant
+        // (and numpy ≥ 1.23 sets the strides to 0 in that case, so we need to check explicitly).
+        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)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rwgk I'd recommend this.

-        // irrelevant)
-        return !negativestrides
-               && (props::inner_stride == Eigen::Dynamic || props::inner_stride == stride.inner()
-                   || (EigenRowMajor ? cols : rows) == 1)
+        // irrelevant). Alternatively, if any dimension size is 0, the strides are not relevant
+        // (and numpy ≥ 1.23 sets the strides to 0 in that case, so we need to check explicitly).
+        if (negativestrides) {
+            return false;
+        }
+        return (rows == 0 || cols == 0) || (props::inner_stride == Eigen::Dynamic || props::inner_stride == stride.inner()
+                || (EigenRowMajor ? cols : rows) == 1)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's good, too.
For the && I think clang-tidy may warn about adding parenthesis, if I understand the suggestion correctly, in this github view.

Copy link
Contributor Author

@mwoehlke-kitware mwoehlke-kitware Jun 29, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Er... I missed @Skylion007's update. However, I think the additional conditional nesting in that version is actually less readable, especially as it further destroys the visual symmetry between the inner/outer tests.

To clarify; the current form is return A && B, where A and B are similar, complicated predicates (e.g. A is really (A1 || A2 || A3)) that used to exactly line up, and are now off by 3 characters. The second proposal is return C || (A && B), with A and B much further out of alignment and multiply nested Boolean operators.

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);
}
Expand Down
7 changes: 5 additions & 2 deletions tests/test_eigen.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -345,10 +345,13 @@ TEST_SUBMODULE(eigen, m) {
},
py::arg{}.noconvert());

// test_issue738
// Issue #738: 1xN or Nx1 2D matrices were neither accepted nor properly copied with an
// test_issue738, test_zero_length
// Issue #738: 1×N or N×1 2D matrices were neither accepted nor properly copied with an
// incompatible stride value on the length-1 dimension--but that should be allowed (without
// requiring a copy!) because the stride value can be safely ignored on a size-1 dimension.
// Similarly, 0×N or N×0 matrices were not accepted--again, these should be allowed since
// they contain no data. This particularly affects numpy ≥ 1.23, which sets the strides to
// 0 if any dimension size is 0.
m.def("iss738_f1",
&adjust_matrix<const Eigen::Ref<const Eigen::MatrixXd> &>,
py::arg{}.noconvert());
Expand Down
7 changes: 7 additions & 0 deletions tests/test_eigen.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -744,6 +744,13 @@ def test_issue738():
)


@pytest.mark.parametrize("func", [m.iss738_f1, m.iss738_f2])
@pytest.mark.parametrize("sizes", [(0, 2), (2, 0)])
def test_zero_length(func, sizes):
"""Ignore strides on a length-0 dimension (even if they would be incompatible length > 1)"""
assert np.all(func(np.zeros(sizes)) == np.zeros(sizes))


def test_issue1105():
"""Issue 1105: 1xN or Nx1 input arrays weren't accepted for eigen
compile-time row vectors or column vector"""
Expand Down