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.

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

Enable numpy math functions to work with symbolic variables. #15039

Conversation

soonho-tri
Copy link
Member

@soonho-tri soonho-tri commented May 12, 2021

#15038


This change is Reviewable

Copy link
Member Author

@soonho-tri soonho-tri left a comment

Choose a reason for hiding this comment

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

+@RussTedrake for feature-review, please.
+@EricCousineau-TRI for platform-review, please.

Reviewable status: LGTM missing from assignees RussTedrake(platform),EricCousineau-TRI(platform) (waiting on @EricCousineau-TRI and @RussTedrake)

@jwnimmer-tri
Copy link
Collaborator

FYI do you want Expression as well as Variable to have these methods?

Copy link
Contributor

@EricCousineau-TRI EricCousineau-TRI left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r1.
Reviewable status: 1 unresolved discussion, LGTM missing from assignees EricCousineau-TRI(platform),RussTedrake(platform) (waiting on @RussTedrake and @soonho-tri)


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

          },
          py::is_operator())
      // We add the following methods (exp, sqrt, sin, cos, tan, arcsin, arccos,

Ah, sorry; if you see the following, it may be possible to retarget the following method to do what you want:

// TODO(eric.cousineau): Per TODO in `symbolic_py.cc`, ensure that this binds
// directly to the class, so it is the same as AutoDiff.
// Adds math function overloads for Expression (ADL free functions from
// `symbolic_expression.h`) for both NumPy methods and `pydrake.math`.
// @param obj
// This is used to register functions or overloads in either (a)
// `pydrake.symbolic` or `pydrake.math`.
template <typename PyObject>
void BindSymbolicMathOverloads(PyObject* obj) {
using symbolic::Expression;
(*obj) // BR
.def("log", &symbolic::log)
.def("abs", &symbolic::abs)
.def("exp", &symbolic::exp)
.def("sqrt", &symbolic::sqrt)
.def("pow",
[](const Expression& base, const Expression& exponent) {
return pow(base, exponent);
})
.def("sin", &symbolic::sin)
.def("cos", &symbolic::cos)
.def("tan", &symbolic::tan)
.def("asin", &symbolic::asin)
.def("acos", &symbolic::acos)
.def("atan", &symbolic::atan)
.def("atan2", &symbolic::atan2)
.def("sinh", &symbolic::sinh)
.def("cosh", &symbolic::cosh)
.def("tanh", &symbolic::tanh)
.def("min", &symbolic::min)
.def("max", &symbolic::max)
.def("ceil", &symbolic::ceil)
.def("floor", &symbolic::floor)
// TODO(eric.cousineau): This is not a NumPy-overridable method using
// dtype=object. Deprecate and move solely into `pydrake.math`.
.def("inv", [](const MatrixX<Expression>& X) -> MatrixX<Expression> {
return X.inverse();
});
}

I'll see if I can do a quick PR

@soonho-tri
Copy link
Member Author

FYI do you want Expression as well as Variable to have these methods?

@jwnimmer-tri , FYI, we've already handled the case:

Screen Shot 2021-05-12 at 11 30 00 AM

@soonho-tri
Copy link
Member Author

I'll see if I can do a quick PR

@EricCousineau-TRI , OK. Please ping me when you open your PR.

Copy link
Contributor

@EricCousineau-TRI EricCousineau-TRI left a comment

Choose a reason for hiding this comment

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

Reviewable status: 1 unresolved discussion, LGTM missing from assignees EricCousineau-TRI(platform),RussTedrake(platform) (waiting on @EricCousineau-TRI and @RussTedrake)


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

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Ah, sorry; if you see the following, it may be possible to retarget the following method to do what you want:

// TODO(eric.cousineau): Per TODO in `symbolic_py.cc`, ensure that this binds
// directly to the class, so it is the same as AutoDiff.
// Adds math function overloads for Expression (ADL free functions from
// `symbolic_expression.h`) for both NumPy methods and `pydrake.math`.
// @param obj
// This is used to register functions or overloads in either (a)
// `pydrake.symbolic` or `pydrake.math`.
template <typename PyObject>
void BindSymbolicMathOverloads(PyObject* obj) {
using symbolic::Expression;
(*obj) // BR
.def("log", &symbolic::log)
.def("abs", &symbolic::abs)
.def("exp", &symbolic::exp)
.def("sqrt", &symbolic::sqrt)
.def("pow",
[](const Expression& base, const Expression& exponent) {
return pow(base, exponent);
})
.def("sin", &symbolic::sin)
.def("cos", &symbolic::cos)
.def("tan", &symbolic::tan)
.def("asin", &symbolic::asin)
.def("acos", &symbolic::acos)
.def("atan", &symbolic::atan)
.def("atan2", &symbolic::atan2)
.def("sinh", &symbolic::sinh)
.def("cosh", &symbolic::cosh)
.def("tanh", &symbolic::tanh)
.def("min", &symbolic::min)
.def("max", &symbolic::max)
.def("ceil", &symbolic::ceil)
.def("floor", &symbolic::floor)
// TODO(eric.cousineau): This is not a NumPy-overridable method using
// dtype=object. Deprecate and move solely into `pydrake.math`.
.def("inv", [](const MatrixX<Expression>& X) -> MatrixX<Expression> {
return X.inverse();
});
}

I'll see if I can do a quick PR

I've filed the following against this PR: soonho-tri#30

Didn't fully vet testing; please let me know if it's worth polishing this approach, and I can try to doc a bit more.

Copy link
Contributor

@EricCousineau-TRI EricCousineau-TRI left a comment

Choose a reason for hiding this comment

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

Reviewable status: 1 unresolved discussion, LGTM missing from assignees EricCousineau-TRI(platform),RussTedrake(platform) (waiting on @RussTedrake and @soonho-tri)


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

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

I've filed the following against this PR: soonho-tri#30

Didn't fully vet testing; please let me know if it's worth polishing this approach, and I can try to doc a bit more.

"fully vet testing" => I ran bazel test and it passes; however, I didn't add new tests

@soonho-tri soonho-tri force-pushed the numpy-math-functions-symbolic-variable branch from 551080c to 9356274 Compare May 12, 2021 16:12
Copy link
Contributor

@EricCousineau-TRI EricCousineau-TRI left a comment

Choose a reason for hiding this comment

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

Reviewable status: LGTM missing from assignees EricCousineau-TRI(platform),RussTedrake(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @EricCousineau-TRI and @RussTedrake)


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

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

"fully vet testing" => I ran bazel test and it passes; however, I didn't add new tests

OK Sweet! I'll push a few more fixes (including fixing CI).

Copy link
Contributor

@EricCousineau-TRI EricCousineau-TRI left a comment

Choose a reason for hiding this comment

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

Reviewable status: 1 unresolved discussion, LGTM missing from assignees EricCousineau-TRI(platform),RussTedrake(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @EricCousineau-TRI, @RussTedrake, and @soonho-tri)

a discussion (no related file):
nit Does this close above referenced issue? (at present, this only references it)


Copy link
Contributor

@EricCousineau-TRI EricCousineau-TRI left a comment

Choose a reason for hiding this comment

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

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


bindings/pydrake/symbolic_types_pybind.h, line 58 at r2 (raw file):

      .def("atan", &symbolic::atan, doc.atan.doc)
      .def("arctan2", &symbolic::atan2, doc.atan2.doc)
      .def("atan2", &symbolic::atan2, doc.atan2.doc)

Working: Significance of each separate overload is opaque. Trying to clarify with docs.

Copy link
Member Author

@soonho-tri soonho-tri left a comment

Choose a reason for hiding this comment

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

-@EricCousineau-TRI
+@soonho-tri . With Eric's commit, I've only added the test cases. So I'll play the role of platform-reviewer.

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

Copy link
Contributor

@EricCousineau-TRI EricCousineau-TRI left a comment

Choose a reason for hiding this comment

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

Reviewable status: 1 unresolved discussion, LGTM missing from assignees soonho-tri(platform),RussTedrake(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @EricCousineau-TRI, @RussTedrake, and @soonho-tri)


bindings/pydrake/symbolic_types_pybind.h, line 58 at r2 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Working: Significance of each separate overload is opaque. Trying to clarify with docs.

OK Further docs will pollute scope of this PR. Opened #15041

Copy link
Contributor

@EricCousineau-TRI EricCousineau-TRI left a comment

Choose a reason for hiding this comment

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

I've strictly timed out for this week; my suggestion is that this is a strict improvement. There's a mess underyling, but that can be resolved via follow-up such as #15041

Reviewable status: 1 unresolved discussion, LGTM missing from assignees soonho-tri(platform),RussTedrake(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @EricCousineau-TRI, @RussTedrake, and @soonho-tri)

Copy link
Contributor

@RussTedrake RussTedrake left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 3 of 4 files at r2, 1 of 1 files at r3.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee soonho-tri(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @soonho-tri)


bindings/pydrake/symbolic_types_pybind.h, line 77 at r3 (raw file):

     // dtype=object. Deprecate and move solely into `pydrake.math`.
      .def(
          "inv",

nit: is this covered by a test? (I couldn't see it)?

Copy link
Contributor

@EricCousineau-TRI EricCousineau-TRI left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 3 files at r4.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee soonho-tri(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @RussTedrake and @soonho-tri)


bindings/pydrake/symbolic_types_pybind.h, line 77 at r3 (raw file):

Previously, RussTedrake (Russ Tedrake) wrote…

nit: is this covered by a test? (I couldn't see it)?

OK Yup! Actually, failures from prior revision was b/c I accidentally dropped this from the bindings 😅

Copy link
Member Author

@soonho-tri soonho-tri left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 2 files at r1, 2 of 4 files at r2, 3 of 3 files at r4.
Reviewable status: 1 unresolved discussion, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @soonho-tri)

a discussion (no related file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

nit Does this close above referenced issue? (at present, this only references it)

I'll check it after merging this PR.


a discussion (no related file):
@EricCousineau-TRI , could you squash everything into a single commit and update this PR? Currently, if we squash this, I'm afraid I'll be the author of all the changes.


@EricCousineau-TRI EricCousineau-TRI force-pushed the numpy-math-functions-symbolic-variable branch from dbe7506 to f31c51c Compare May 15, 2021 21:17
Copy link
Contributor

@EricCousineau-TRI EricCousineau-TRI left a comment

Choose a reason for hiding this comment

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

Reviewable status: 1 unresolved discussion (waiting on @soonho-tri)

a discussion (no related file):

Previously, soonho-tri (Soonho Kong) wrote…

@EricCousineau-TRI , could you squash everything into a single commit and update this PR? Currently, if we squash this, I'm afraid I'll be the author of all the changes.

Done!


@EricCousineau-TRI EricCousineau-TRI changed the title Enable numpy math functions to work with symbolic variables. Extend support of numpy math functions to work with symbolic Variables. May 15, 2021
@EricCousineau-TRI EricCousineau-TRI changed the title Extend support of numpy math functions to work with symbolic Variables. Enable numpy math functions to work with symbolic variables. May 15, 2021
@soonho-tri soonho-tri merged commit 2d57227 into RobotLocomotion:master May 15, 2021
@soonho-tri soonho-tri deleted the numpy-math-functions-symbolic-variable branch May 15, 2021 23:52
rpoyner-tri pushed a commit to rpoyner-tri/drake that referenced this pull request May 16, 2021
…comotion#15039)

* Enable numpy math functions to work with symbolic variables.

RobotLocomotion#15038

Co-authored-by: Soonho Kong <soonho.kong@tri.global>
rpoyner-tri added a commit that referenced this pull request May 17, 2021
* DNM Disable CI

* notes: clean up the first few sections

* wip

* wip

* wip

* wip

* wip

* wip

* wip

* wip: first pass done

* Factor out to h2 and sort the Build Dependencies

* Remove non-notable changes (refactorings, docs)

* Move APT site to announcements

* Punt hydroelastic viz change into dynamics section

Probably all of the hydroelatic changes should be culled, but having
them all in once place is a start.

* Prioritize and reword the breaking changes section

* Note AVX breaking change

* Fix some pydrake typos

Remove non-notable (unit test) change

* quadrotor example: add getter for geometry frame (#15046)

* quadrotor example: add getter for geometry frame

* Enable numpy math functions to work with symbolic variables. (#15039)

* Enable numpy math functions to work with symbolic variables.

#15038

Co-authored-by: Soonho Kong <soonho.kong@tri.global>

* Remove deprecated code 2021-04-21 (#15050)

Missed previously, perhaps owing to idiosyncratic date.

* drake::multibody::MultiBodyPlant::CalcCenterOfMassPosition
  * all overloads

* wip

* wip

* integrate new commits from merge of master

* Merge remote-tracking branch 'upstream/master' into release_notes-v0.30.0

* Fix CaMel case typo

* wip

* set date

* Revert "DNM Disable CI"

This reverts commit 608e8ec.

* wip
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants