Fix arrays with zero-size dimensions

[mwoehlke-kitware]

Description

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. (This test likely would have caught the behavior change with numpy 1.23.)

This issue was previously fixed by RobotLocomotion#38 which does not appear to have ever been upstreamed. (The tests come from that PR.) However, that fix broke with numpy 1.23.

Suggested changelog entry:

* Arrays with a dimension of size 0 are now properly converted to dynamic Eigen
  matrices.

[rwgk]

I initiated Google-global testing for this PR. I'll post the results when I have them, with some luck in ~5 hours.

[Skylion007]

Thanks for adding a unit test. Nit: this seems like it might be a good test case to parameterize using `@pytest.mark.parameterize'. Otherwise, this looks good.

[mwoehlke-kitware]

Strictly speaking, you should thank @EricCousineau-TRI for the tests 🙂; I just tweaked the ones he added in RobotLocomotion#38. (But, yeah, more tests == more good. Especially as Eric's original tests, which didn't cover all possible combinations of row-vs-column order and zero size, missed the numpy 1.23 breakage, which is why there are four rather than two. BTW, I also verified that this suite does catch the numpy 1.23 breakage, even with Eric's previous patch which only worked with numpy < 1.23.)

this seems like it might be a good test case to parameterize using @pytest.mark.parameterize.

Do you mean something like this?

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

[Skylion007]

@mwoehlke-kitware Exactly!

[rwgk]

+1

The main advantage is that all tests run even if one fails. That's very useful especially when looking at CI logs, when it's not easy to rerun interactively to see what else fails.

[rwgk]

You can double parameterize (on (m.iss738_1, m.iss738_2)), then all tests run even if any fails.

[rwgk]

Example: test_const_name.py

[Skylion007]

Agree with @rwgk, I was debating whether to parameterize on function names too, but this is pretty readable with that shorthand actually.

[mwoehlke-kitware]

Done.

[rwgk]

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)

[Skylion007]

@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)

[rwgk]

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.

[mwoehlke-kitware]

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.

[rwgk]

The global testing finished much faster than expected, and it passed.

Core pybind11 changes trigger millions of tests, but it turns out isolated changes to eigen.h only trigger a few thousand. (Interesting to know.)

[rwgk]

Thanks Matthew!