pydrake autodiff, symbolic: Add initial tests for scalar and vectorized math

[EricCousineau-TRI]

Relates #8116 and #8315

The purpose of this PR is to (a) lock down elucidate behavior with NumPy arrays and (b) provide a basis for seeing how the solution to #8315 will affect the behavior.

---

This change is 

[EricCousineau-TRI]

+@soonho-tri for feature review, please.
(Will add either Sherm or Jeremy for platform review.)

---

Review status: 0 of 8 files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable

[soonho-tri]

Check point.

---

Reviewed 4 of 8 files at r1.
Review status: 4 of 8 files reviewed at latest revision, all discussions resolved.

---

bindings/pydrake/autodiffutils_py.cc, line 69 at r1 (raw file):

         }, py::is_operator());

    // Add overloads for `sin` and `cos`.

BTW, this comment is outdated now.

---

bindings/pydrake/symbolic_py.cc, line 40 at r1 (raw file):

      .def("__hash__",
           [](const Variable& self) { return std::hash<Variable>{}(self); })
      .def("__copy__",

Could you explain why we need this? If it's necessary, I will update other classes here (i.e. Monomial, Polynomial) to have __copy__.

---

bindings/pydrake/util/wrap_pybind.h, line 14 at r1 (raw file):

namespace pydrake {

// Defines a function in module `m`, and mirrors to module `mirror` for

Could you update the description to include From and To?

---

bindings/pydrake/util/wrap_pybind.h, line 15 at r1 (raw file):

Throws an error...

I think we do not do this anymore? Is it worth documenting why we stopped doing the check?
Also, can we have a Doxygen section for def method?

---

bindings/pydrake/util/wrap_pybind.h, line 22 at r1 (raw file):

  MirrorDef(From* m, To* mirror)
      : m_(m), mirror_(mirror) {}

BTW, missing DRAKE_NO_COPY_NO_MOVE_NO_ASSIGN or DRAKE_DEFAULT_COPY_AND_MOVE_AND_ASSIGN.

---

bindings/pydrake/util/wrap_pybind.h, line 31 at r1 (raw file):

 private:
  From* m_{};

BTW, make those pointers to be const?

  From* const m_{};
  To* const mirror_{};

---

Comments from Reviewable

[soonho-tri]

First pass complete. Also please assign it to a platform-reviewer.

---

Reviewed 4 of 8 files at r1.
Review status: all files reviewed at latest revision, 6 unresolved discussions.

---

bindings/pydrake/test/autodiffutils_test.py, line 37 at r1 (raw file):

                self._check_scalar(a, b)
        else:
            self.assertTrue(np.allclose(actual, expected))

I'm not sure that the use of np.allclose is appropriate when it's (always?) the case that the type of actual and expected is np.array of np.bool_.

---

bindings/pydrake/test/math_test_util.py, line 12 at r1 (raw file):

    def reformat(self, scalar):
        # Reformats a scalar to the given form.

BTW, to my standard, changing 2 => [2, 2] is something more than reformatting. I'd be great if we can find a better name (i.e. make_T.. but it's too C++-ish?) with a proper documentation.

---

bindings/pydrake/test/math_test_util.py, line 15 at r1 (raw file):

        raise NotImplemented

    def check_value(self, actual, expected_scalar):

Could you add a documentation for this?

# Checks if `actual` is `expected_scalar`.

---

bindings/pydrake/test/math_test_util.py, line 18 at r1 (raw file):

        raise NotImplemented

    def check_logical(self, actual, expected_scalar):

I guess the signature should be def check_logical(self, func, a, b, expected_scalar) instead? Can you add a documentation for this? I think the name check_logical is not so much self-explanatory.

---

bindings/pydrake/test/math_test_util.py, line 52 at r1 (raw file):

        expected = self.reformat(expected_scalar)
        self._check_value_impl(func(a, b), expected)
        self._check_value_impl(func(a, b.value()), expected)

From .value(), I assume that a and b are of numpy array of AutoDiffXd. Can we still use ScalarMath and VectorizedMath classes with symbolic? Maybe it's because of my lack of understanding of what check_logical is supposed to do.

---

bindings/pydrake/test/math_test_util.py, line 80 at r1 (raw file):

        self._check_value_impl(actual, expected)

    def _array_to_float(self, a):

This method converts an array of AutoDiffXd to an array of float, correct? Then, I think the name _array_to_float is misleading.

---

bindings/pydrake/test/math_test_util.py, line 84 at r1 (raw file):

    def check_logical(self, func, a, b, expected_scalar):
        # See above.

BTW, meant # See check_logical in ScalarMath?

---

common/test/autodiff_overloads_test.cc, line 55 at r1 (raw file):

  AutoDiffXd y(3);
  EXPECT_EQ((x * y).value(), 6);

Is it worth adding EXPECT_EQ((x * y).derivatives().size(), 3); as well here?

---

Comments from Reviewable

[EricCousineau-TRI]

bindings/pydrake/symbolic_py.cc, line 40 at r1 (raw file):

Previously, soonho-tri (Soonho Kong) wrote…

Could you explain why we need this? If it's necessary, I will update other classes here (i.e. Monomial, Polynomial) to have __copy__.

OK It was just a local need for testing.
In the in-place operator tests, you had assigned a value by reference (not value), e.g.:

e = e_x   # Mutates global `e_x`
e += e_y
self.assertEquals(e, x + y)  # Works

# Some other test
self.assertEquals(e_x + e_y, x+ y)  # Now fails, because `e_x` has been mutated via an alias in `e`

I believe this worked before because either (a) unittest was restoring local values or (b) when I changed the scoping, += allowed some sort of reassignment (e.g. for type changes, x += e_y might have promoted x from Variable to Expression).

Didn't really try tracking down why specifically, but the issue was solvable by using e = copy(e_x) to ensure unique instances.

I'm not sure if it's necessary for other things, but perhaps it would be useful?

---

Comments from Reviewable

[soonho-tri]

Review status: all files reviewed at latest revision, 14 unresolved discussions.

---

bindings/pydrake/symbolic_py.cc, line 40 at r1 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

OK It was just a local need for testing.
In the in-place operator tests, you had assigned a value by reference (not value), e.g.:

e = e_x   # Mutates global `e_x`
e += e_y
self.assertEquals(e, x + y)  # Works

# Some other test
self.assertEquals(e_x + e_y, x+ y)  # Now fails, because `e_x` has been mutated via an alias in `e`

I believe this worked before because either (a) unittest was restoring local values or (b) when I changed the scoping, += allowed some sort of reassignment (e.g. for type changes, x += e_y might have promoted x from Variable to Expression).

Didn't really try tracking down why specifically, but the issue was solvable by using e = copy(e_x) to ensure unique instances.

I'm not sure if it's necessary for other things, but perhaps it would be useful?

Yeah, I had the issue. Good to know that it's mainly for the testing. I'll add more if I find use cases in the future. Thank you.

---

Comments from Reviewable

[EricCousineau-TRI]

Review status: 1 of 8 files reviewed at latest revision, 9 unresolved discussions.

---

bindings/pydrake/autodiffutils_py.cc, line 69 at r1 (raw file):

Previously, soonho-tri (Soonho Kong) wrote…

BTW, this comment is outdated now.

Done.

---

bindings/pydrake/test/autodiffutils_test.py, line 37 at r1 (raw file):

Previously, soonho-tri (Soonho Kong) wrote…

I'm not sure that the use of np.allclose is appropriate when it's (always?) the case that the type of actual and expected is np.array of np.bool_.

Done.

---

bindings/pydrake/util/wrap_pybind.h, line 14 at r1 (raw file):

Previously, soonho-tri (Soonho Kong) wrote…

Could you update the description to include From and To?

Done.

---

bindings/pydrake/util/wrap_pybind.h, line 22 at r1 (raw file):

Previously, soonho-tri (Soonho Kong) wrote…

BTW, missing DRAKE_NO_COPY_NO_MOVE_NO_ASSIGN or DRAKE_DEFAULT_COPY_AND_MOVE_AND_ASSIGN.

Done.

---

bindings/pydrake/util/wrap_pybind.h, line 31 at r1 (raw file):

Previously, soonho-tri (Soonho Kong) wrote…

BTW, make those pointers to be const?

  From* const m_{};
  To* const mirror_{};

Done.

---

common/test/autodiff_overloads_test.cc, line 55 at r1 (raw file):

Previously, soonho-tri (Soonho Kong) wrote…

Is it worth adding EXPECT_EQ((x * y).derivatives().size(), 3); as well here?

Done.

---

bindings/pydrake/test/math_test_util.py, line 12 at r1 (raw file):

Previously, soonho-tri (Soonho Kong) wrote…

BTW, to my standard, changing 2 => [2, 2] is something more than reformatting. I'd be great if we can find a better name (i.e. make_T.. but it's too C++-ish?) with a proper documentation.

Done. Figgered that algebra may be a bit more descriptive. Wasn't sure whether to state array or vectorized algebra, though.

---

bindings/pydrake/test/math_test_util.py, line 15 at r1 (raw file):

Previously, soonho-tri (Soonho Kong) wrote…

Could you add a documentation for this?

# Checks if `actual` is `expected_scalar`.

Done.

---

bindings/pydrake/test/math_test_util.py, line 18 at r1 (raw file):

Previously, soonho-tri (Soonho Kong) wrote…

I guess the signature should be def check_logical(self, func, a, b, expected_scalar) instead? Can you add a documentation for this? I think the name check_logical is not so much self-explanatory.

Done.

---

bindings/pydrake/test/math_test_util.py, line 52 at r1 (raw file):

Previously, soonho-tri (Soonho Kong) wrote…

From .value(), I assume that a and b are of numpy array of AutoDiffXd. Can we still use ScalarMath and VectorizedMath classes with symbolic? Maybe it's because of my lack of understanding of what check_logical is supposed to do.

Done.

---

bindings/pydrake/test/math_test_util.py, line 80 at r1 (raw file):

Previously, soonho-tri (Soonho Kong) wrote…

This method converts an array of AutoDiffXd to an array of float, correct? Then, I think the name _array_to_float is misleading.

Done.

---

bindings/pydrake/test/math_test_util.py, line 84 at r1 (raw file):

Previously, soonho-tri (Soonho Kong) wrote…

BTW, meant # See check_logical in ScalarMath?

Done.

---

Comments from Reviewable

[EricCousineau-TRI]

+@sherm1 for platform review, please (given that Soonho is feature reviewing)

---

Review status: 1 of 8 files reviewed at latest revision, 8 unresolved discussions.

---

Comments from Reviewable

[EricCousineau-TRI]

bindings/pydrake/util/wrap_pybind.h, line 15 at r1 (raw file):

Previously, soonho-tri (Soonho Kong) wrote…

Throws an error...

I think we do not do this anymore? Is it worth documenting why we stopped doing the check?
Also, can we have a Doxygen section for def method?

Done.

---

Comments from Reviewable

[soonho-tri]

Reviewed 4 of 8 files at r2, 4 of 4 files at r3.
Review status: all files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable

[soonho-tri]

Review status: all files reviewed at latest revision, 2 unresolved discussions.

---

bindings/pydrake/test/algebra_test_util.py, line 11 at r3 (raw file):

class BaseAlgebra(object):
    # Base class for defining scalar or vectorized (array) algebra and math.
    # checks on custom types that have numeric relations to `float`.

BTW, nit, capitalize checks -> Checks.

---

bindings/pydrake/test/algebra_test_util.py, line 54 at r3 (raw file):

    def __init__(self, check_value_impl, scalar_to_float):
        BaseAlgebra.__init__(self, check_value_impl, scalar_to_float)
        # Math functions:

BTW, missing self.log = drake_math.log? If this is the case, we need one for the below VectorizedAlgebra.__init__ (+ tests)

---

Comments from Reviewable

[EricCousineau-TRI]

Review status: 3 of 8 files reviewed at latest revision, 2 unresolved discussions.

---

bindings/pydrake/test/algebra_test_util.py, line 11 at r3 (raw file):

Previously, soonho-tri (Soonho Kong) wrote…

BTW, nit, capitalize checks -> Checks.

Done.

---

bindings/pydrake/test/algebra_test_util.py, line 54 at r3 (raw file):

Previously, soonho-tri (Soonho Kong) wrote…

BTW, missing self.log = drake_math.log? If this is the case, we need one for the below VectorizedAlgebra.__init__ (+ tests)

Done.

---

Comments from Reviewable

[soonho-tri]

---

Reviewed 5 of 6 files at r4.
Review status: all files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable

[EricCousineau-TRI]

BTW @RussTedrake Can I ask if there are any other expressions that you're finding that you would like me to guarantee behavior for? (scalar and array algebra-wise)

[RussTedrake]

not off the top of my head. but thanks!

[sherm1]

Platform . Reviewed C++ parts, cursory look at Python code but since we've got four platform reviewers on this I'll deem it adequately reviewed!

---

Reviewed 3 of 8 files at r2, 1 of 4 files at r3, 5 of 5 files at r4.
Review status: all files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable