py math: Disentangle .eigen_geometry, .math, .autodiff, and .symbolic #11384

EricCousineau-TRI · 2019-05-06T00:31:19Z

This change is

EricCousineau-TRI

+@sammy-tri for feature review, please.

It's not entirely clean, but I'd rather bump this closer towards the related PRs and resolve later (hopefully once NumPy UFuncs become a real thing).

Reviewable status: LGTM missing from assignee sammy-tri(platform), needs at least two assigned reviewers (waiting on @sammy-tri)

sammy-tri

+@jwnimmer-tri for platform review

Reviewed 10 of 10 files at r1.
Reviewable status: LGTM missing from assignee jwnimmer-tri(platform) (waiting on @jwnimmer-tri)

jwnimmer-tri

Reviewed 10 of 10 files at r1.
Reviewable status: 8 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform) (waiting on @EricCousineau-TRI)

bindings/pydrake/autodiff_types_pybind.h, line 25 at r1 (raw file):

template <typename PyObject>
void BindAutoDiffOverloads(PyObject* obj) {

Unclear what a valid (sensible / acceptable) value for obj might be.

bindings/pydrake/autodiff_types_pybind.h, line 52 at r1 (raw file):

      .def("inv", [](const MatrixX<AutoDiffXd>& X) -> MatrixX<AutoDiffXd> {
        return X.inverse();
      });

Unclear what names should / should not appear in this list. What is it compatible with?

bindings/pydrake/BUILD.bazel, line 183 at r1 (raw file):

        ":module_py",
        "//bindings/pydrake:autodiffutils_py",
        "//bindings/pydrake:symbolic_py",

I can't figure out why these two new lines should be here.

bindings/pydrake/BUILD.bazel, line 443 at r1 (raw file):

    deps = [
        ":algebra_test_util_py",
        ":math_py",

I can't figure out why this new line should be here.

bindings/pydrake/symbolic_types_pybind.h, line 29 at r1 (raw file):

template <typename PyObject>
void BindSymbolicOverloads(PyObject obj) {

Unclear what a valid (sensible / acceptable) value for obj might be.

bindings/pydrake/symbolic_types_pybind.h, line 55 at r1 (raw file):

      .def("inv", [](const MatrixX<Expression>& X) -> MatrixX<Expression> {
        return X.inverse();
      });

Unclear what names should / should not appear in this list. What is it compatible with? Or should it just be all of the ADL unary and binary named functions in symbolic_expression.h?

bindings/pydrake/common/BUILD.bazel, line 135 at r1 (raw file):

        ":module_py",
        "//bindings/pydrake:autodiffutils_py",
        "//bindings/pydrake:symbolic_py",

I can't figure out why these two new lines should be here.

bindings/pydrake/common/wrap_pybind.h, line 18 at r1 (raw file):

namespace pydrake {

/// Defines a function in object `a` and mirrors `def` calls to object `b`.

We might as well just mark this deprecated? The header is public and keeping it here seems easy enough. Or talk me through your thought process on yanking it without deprecation?

EricCousineau-TRI

Reviewable status: 8 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform) (waiting on @jwnimmer-tri and @sammy-tri)

bindings/pydrake/autodiff_types_pybind.h, line 25 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Unclear what a valid (sensible / acceptable) value for obj might be.

Done.

bindings/pydrake/autodiff_types_pybind.h, line 52 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Unclear what names should / should not appear in this list. What is it compatible with?

Done.

bindings/pydrake/BUILD.bazel, line 183 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

I can't figure out why these two new lines should be here.

OK For accessing BindAutoDiffOverloads.

bindings/pydrake/BUILD.bazel, line 443 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

I can't figure out why this new line should be here.

Done. Bad IWYU - oops!

bindings/pydrake/symbolic_types_pybind.h, line 29 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Unclear what a valid (sensible / acceptable) value for obj might be.

Done.

bindings/pydrake/symbolic_types_pybind.h, line 55 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Unclear what names should / should not appear in this list. What is it compatible with? Or should it just be all of the ADL unary and binary named functions in symbolic_expression.h?

Done.

bindings/pydrake/common/BUILD.bazel, line 135 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

I can't figure out why these two new lines should be here.

Done. Cruft from WIP branch - oops!

bindings/pydrake/common/wrap_pybind.h, line 18 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

We might as well just mark this deprecated? The header is public and keeping it here seems easy enough. Or talk me through your thought process on yanking it without deprecation?

Got trigger happy for deleting code.
That being said, it's very esoteric, and only accessible via Bazel build; I highly doubt anyone will have ever wanted to touch it, much less still be using it.
Would prefer to leave it behind, but will bring it back if you're concerned.

jwnimmer-tri

Reviewed 4 of 4 files at r2.
Reviewable status: 3 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform) (waiting on @EricCousineau-TRI and @jwnimmer-tri)

bindings/pydrake/BUILD.bazel, line 183 at r1 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

OK For accessing BindAutoDiffOverloads.

But BindAutoDiffOverloads is in cc_deps //bindings/pydrake:autodiff_types_pybind not py_deps //bindings/pydrake:autodiffutils_py? I think the new cc_deps are good, but I don't yet understand the new py_deps. Maybe I'm missing something obvious...

bindings/pydrake/common/wrap_pybind.h, line 18 at r1 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Got trigger happy for deleting code.
That being said, it's very esoteric, and only accessible via Bazel build; I highly doubt anyone will have ever wanted to touch it, much less still be using it.
Would prefer to leave it behind, but will bring it back if you're concerned.

As long as you've thought about it, I can let it go.

jwnimmer-tri

Reviewable status: 2 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform) (waiting on @EricCousineau-TRI)

bindings/pydrake/autodiff_types_pybind.h, line 52 at r1 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Done.

It says "Adds math function overloads for AutoDiffXd" but what is the charter of this function? Is that math functions as in python's import math, or is it https://en.cppreference.com/w/cpp/header/cmath, or is it "common/autodiff.h's ADL free functions" or is it a low bar, more like "put anything here you like, as long as it relates to AutoDiffXd in any way at all?".

EricCousineau-TRI

Reviewable status: 2 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @jwnimmer-tri and @sammy-tri)

bindings/pydrake/autodiff_types_pybind.h, line 52 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

It says "Adds math function overloads for AutoDiffXd" but what is the charter of this function? Is that math functions as in python's import math, or is it https://en.cppreference.com/w/cpp/header/cmath, or is it "common/autodiff.h's ADL free functions" or is it a low bar, more like "put anything here you like, as long as it relates to AutoDiffXd in any way at all?".

Ah... sorry, I see; it's meant to be for ADL free functions.
Done. Realized that I've made a bit of a mess for this stuff (per the new TODOs)... New deprecations soon to come lol

bindings/pydrake/BUILD.bazel, line 183 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

But BindAutoDiffOverloads is in cc_deps //bindings/pydrake:autodiff_types_pybind not py_deps //bindings/pydrake:autodiffutils_py? I think the new cc_deps are good, but I don't yet understand the new py_deps. Maybe I'm missing something obvious...

Ah, sorry - the py_deps are also necessary since *_types_pybind bind overloads that reference those objects. While they could be bound without those deps, they are necessary to be callable.

(At some point, I'd like to make the C++-required Python deps to be transitively incorporated directly... I have a TODO for that in pybind.bzl I think)

Module loading order is no longer an issue

EricCousineau-TRI

Reviewable status: 2 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @jwnimmer-tri and @sammy-tri)

bindings/pydrake/BUILD.bazel, line 183 at r1 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Ah, sorry - the py_deps are also necessary since *_types_pybind bind overloads that reference those objects. While they could be bound without those deps, they are necessary to be callable.

(At some point, I'd like to make the C++-required Python deps to be transitively incorporated directly... I have a TODO for that in pybind.bzl I think)

Er, ah, forgot to explicitly import those modules in the Cc module. Hopefully it's more clear now.

jwnimmer-tri

It might be worth giving Sam time for another review pass.

Reviewed 12 of 12 files at r3.
Reviewable status: commits need curation (https://drake.mit.edu/reviewable.html#curated-commits)

bindings/pydrake/BUILD.bazel, line 183 at r1 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Er, ah, forgot to explicitly import those modules in the Cc module. Hopefully it's more clear now.

Yup, the missing import was the confusion; thanks!

sammy-tri

Reviewed 1 of 4 files at r2, 12 of 12 files at r3.
Reviewable status: commits need curation (https://drake.mit.edu/reviewable.html#curated-commits)

EricCousineau-TRI mentioned this pull request May 6, 2019

py mbp: Expose autodiff and symbolic scalar types #11240

Closed

11 tasks

EricCousineau-TRI assigned sammy-tri May 6, 2019

EricCousineau-TRI commented May 6, 2019

View reviewed changes

EricCousineau-TRI force-pushed the issue/11240_dep_math_cleanup branch 2 times, most recently from 52322a4 to e544d84 Compare May 6, 2019 16:11

sammy-tri assigned jwnimmer-tri May 6, 2019

sammy-tri approved these changes May 6, 2019

View reviewed changes

jwnimmer-tri requested changes May 6, 2019

View reviewed changes

EricCousineau-TRI force-pushed the issue/11240_dep_math_cleanup branch 2 times, most recently from 8059976 to fb406b1 Compare May 6, 2019 23:23

EricCousineau-TRI commented May 6, 2019

View reviewed changes

jwnimmer-tri reviewed May 7, 2019

View reviewed changes

jwnimmer-tri requested changes May 7, 2019

View reviewed changes

EricCousineau-TRI force-pushed the issue/11240_dep_math_cleanup branch from fb406b1 to ee0319f Compare May 7, 2019 02:12

EricCousineau-TRI commented May 7, 2019

View reviewed changes

EricCousineau-TRI added 2 commits May 6, 2019 22:14

py math: Disentangle .eigen_geometry, .math, .autodiff, and .symbolic

e91a5da

Remove redundant math_overloads_* tests

51568be

Module loading order is no longer an issue

EricCousineau-TRI force-pushed the issue/11240_dep_math_cleanup branch from ee0319f to 51568be Compare May 7, 2019 02:14

EricCousineau-TRI commented May 7, 2019

View reviewed changes

jwnimmer-tri approved these changes May 7, 2019

View reviewed changes

sammy-tri approved these changes May 7, 2019

View reviewed changes

EricCousineau-TRI added the status: commits are properly curated https://drake.mit.edu/reviewable.html#curated-commits label May 7, 2019

EricCousineau-TRI merged commit 1dbe73b into RobotLocomotion:master May 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

py math: Disentangle .eigen_geometry, .math, .autodiff, and .symbolic #11384

py math: Disentangle .eigen_geometry, .math, .autodiff, and .symbolic #11384

EricCousineau-TRI commented May 6, 2019 •

edited by sherm1

Loading

EricCousineau-TRI left a comment

sammy-tri left a comment

jwnimmer-tri left a comment

EricCousineau-TRI left a comment

jwnimmer-tri left a comment

jwnimmer-tri left a comment

EricCousineau-TRI left a comment

EricCousineau-TRI left a comment

jwnimmer-tri left a comment

sammy-tri left a comment

py math: Disentangle .eigen_geometry, .math, .autodiff, and .symbolic #11384

py math: Disentangle .eigen_geometry, .math, .autodiff, and .symbolic #11384

Conversation

EricCousineau-TRI commented May 6, 2019 • edited by sherm1 Loading

EricCousineau-TRI left a comment

Choose a reason for hiding this comment

sammy-tri left a comment

Choose a reason for hiding this comment

jwnimmer-tri left a comment

Choose a reason for hiding this comment

EricCousineau-TRI left a comment

Choose a reason for hiding this comment

jwnimmer-tri left a comment

Choose a reason for hiding this comment

jwnimmer-tri left a comment

Choose a reason for hiding this comment

EricCousineau-TRI left a comment

Choose a reason for hiding this comment

EricCousineau-TRI left a comment

Choose a reason for hiding this comment

jwnimmer-tri left a comment

Choose a reason for hiding this comment

sammy-tri left a comment

Choose a reason for hiding this comment

EricCousineau-TRI commented May 6, 2019 •

edited by sherm1

Loading