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

pydrake math: Ensure that .multiply preserves input shape #13886

Conversation

EricCousineau-TRI
Copy link
Contributor

@EricCousineau-TRI EricCousineau-TRI commented Aug 17, 2020

Resolves #13885


This change is Reviewable

Copy link
Contributor Author

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

+@kunimatsu-tri for feature review, please!

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

@kunimatsu-tri
Copy link
Contributor

CI fails :(
/usr/bin/ld.gold: error: bazel-out/k8-opt/bin/bindings/pydrake/systems/_objs/framework.so/framework_py_systems.pic.o: multiple definition of 'drake::pydrake::WrapToMatchInputShape(pybind11::handle)'

Copy link
Contributor

@kunimatsu-tri kunimatsu-tri left a comment

Choose a reason for hiding this comment

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

Besides the tiny details of pybind and CI failures, the overall logic and the tests :lgtm:

Reviewed 8 of 8 files at r1.
Reviewable status: needs platform reviewer assigned, needs at least two assigned reviewers

Copy link
Contributor Author

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

Fixed - thanks!!!
+@sherm1 for review, please!

Reviewable status: LGTM missing from assignee sherm1(platform) (waiting on @kunimatsu-tri and @sherm1)

Copy link
Contributor Author

@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 assignee sherm1(platform) (waiting on @kunimatsu-tri and @sherm1)


bindings/pydrake/common/eigen_pybind.h, line 40 at r2 (raw file):

}

/// Wraps a function (or method) to reshape the output to be the same as a

nit This is wrong. Should update this to say "instance method".

Copy link
Contributor

@kunimatsu-tri kunimatsu-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 1 of 1 files at r2.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee sherm1(platform) (waiting on @sherm1)

Copy link
Member

@sherm1 sherm1 left a comment

Choose a reason for hiding this comment

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

Everything here looks fine pending a few typos. My concern is what's (maybe) not here. Because this has to be done manually for each vector-multiplying object, I wonder whether some may have been missed here or how some future programmer will know this needs to be done for some classes that get wrapped later? Can you add some advice to the pybinding documentation discussing this issue? It seems like something that must now be considered when wrapping any new class, or possibly adding new multiply functionality to an existing class.
Pending that, platform :lgtm:

Reviewed 7 of 8 files at r1, 1 of 1 files at r2.
Reviewable status: 4 unresolved discussions (waiting on @EricCousineau-TRI)


bindings/pydrake/common/init.py, line 11 at r2 (raw file):

def _wrap_to_match_input_shape(f):
    # See dosctring for `WrapToMatchInputShape` in `eigen_pybind.h` for more

nit dosctring -> docstring


bindings/pydrake/common/init.py, line 15 at r2 (raw file):

    # N.B. We cannot use `inspect.ismethod` on pybind11 methods currently, nor
    # can we use `inspect.Signature` due to the fact that pybind11's instance
    # method is not insepctable for overloads.

nit insepctable -> inspectable


bindings/pydrake/common/eigen_pybind.h, line 42 at r2 (raw file):

/// Wraps a function (or method) to reshape the output to be the same as a
/// given input. For simplicity, the function `func` must be an instance method
/// from pybind11, and the input should the first (and only) argument to

nit "should the" -> "should be the"

Copy link
Contributor Author

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

Added overview docs, PTAL!

Reviewable status: :shipit: complete! all discussions resolved, LGTM from assignees kunimatsu-tri,sherm1(platform) (waiting on @kunimatsu-tri and @sherm1)


bindings/pydrake/common/init.py, line 11 at r2 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

nit dosctring -> docstring

Done.


bindings/pydrake/common/init.py, line 15 at r2 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

nit insepctable -> inspectable

Done.


bindings/pydrake/common/eigen_pybind.h, line 40 at r2 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

nit This is wrong. Should update this to say "instance method".

Done.


bindings/pydrake/common/eigen_pybind.h, line 42 at r2 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

nit "should the" -> "should be the"

Done.

Copy link
Member

@sherm1 sherm1 left a comment

Choose a reason for hiding this comment

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

Great, thanks! One typo. Spurious CI failure?

Reviewed 3 of 3 files at r3.
Reviewable status: 1 unresolved discussion (waiting on @EricCousineau-TRI)


bindings/pydrake/pydrake_doxygen.h, line 478 at r3 (raw file):

Certain bound methods, like `RigidTransform.multiply()`, will have overloads
that can multiply and return (a) other `RigidTransform` instances, (b) vectors,
or (c) matrices (repersenting a list of vectors).

nit: repersenting -> representing

Copy link
Contributor Author

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

Yup, looked spurious!

Reviewable status: :shipit: complete! all discussions resolved, LGTM from assignees kunimatsu-tri,sherm1(platform) (waiting on @sherm1)


bindings/pydrake/pydrake_doxygen.h, line 478 at r3 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

nit: repersenting -> representing

Done. Oops!

@EricCousineau-TRI
Copy link
Contributor Author

Will merge on first non-trivial passing CI job.

@EricCousineau-TRI EricCousineau-TRI merged commit 1ea9bbf into RobotLocomotion:master Aug 19, 2020
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.

pydrake math: .multiply shouldn't lose shape information
3 participants