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

[py trajectories] Adjust Clone and MakeDerivative for unique_ptr #22399

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jwnimmer-tri
Copy link
Collaborator

@jwnimmer-tri jwnimmer-tri commented Jan 6, 2025

Towards #5842 and #21968.

The situation with Trajectory is a bit different that some of our other similar changes to bindings for those issues. In most other cases, the only trouble comes from Clone, but with Trajectory we also have an instance factory method MakeDerivative() that also needs a solution. So in this case, we'll use the decorator pattern to solve the ownership problem (via internal::WrappedTrajectory) instead of trying to deal with Clone directly.


This change is Reviewable

@jwnimmer-tri jwnimmer-tri added priority: low release notes: none This pull request should not be mentioned in the release notes labels Jan 6, 2025
@jwnimmer-tri jwnimmer-tri force-pushed the pydrake-trajectory-clone branch 2 times, most recently from 2c6d235 to fe6a6f5 Compare January 6, 2025 15:38
@jwnimmer-tri
Copy link
Collaborator Author

+@rpoyner-tri for feature review, please? This is a companion to #22353. Or maybe is there someone else who we could ramp up on bindings details like this to grow the feature reviewer pool?

Copy link
Contributor

@rpoyner-tri rpoyner-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 7 of 7 files at r1, all commit messages.
Reviewable status: 3 unresolved discussions, needs at least two assigned reviewers


bindings/pydrake/trajectories_py.cc line 205 at r1 (raw file):

            captured_py_traj = py::none();
          });
      // Wrap it in a unique_ptr to meet out required return signature.

typo

Suggestion:

our

bindings/pydrake/trajectories_py.cc line 205 at r1 (raw file):

            captured_py_traj = py::none();
          });
      // Wrap it in a unique_ptr to meet out required return signature.

nit This comment doesn't quite do it for me. My first reading missed the crucial role of WrappedTrajectory in this particular caper.

Maybe instead: "Stuff the shared pointer inside a WrappedTrajectory, and return that via unique_ptr to meet our required return signature."


bindings/pydrake/trajectories_py.cc line 223 at r1 (raw file):

      bool used_legacy_clone = true;
      auto make_python_deepcopy = [&]() -> py::object {
        PYBIND11_OVERLOAD_INT(py::object, Trajectory<T>, "Clone");

nit similar to elsewhere, it's probably useful to point out that this macro lets control fall through if there is no suitable binding.

@jwnimmer-tri jwnimmer-tri force-pushed the pydrake-trajectory-clone branch from fe6a6f5 to 2c7caf5 Compare January 8, 2025 00:55
Because we allow implementations of Trajectory as Python subclasses,
we cannot assume that the deleter associated with a call to Clone is
`delete MyClass`, so we now adjust our PyTrajectory override logic to
wrap its return value in a WrappedTrajectory. The only reason the
unique_ptr used to work is Drake's custom fork or pybind11 with evil
hacks, which will be going away soon.

We also now warn Python subclasses to implement the canonical spelling
of the __deepcopy__ method, instead of overriding the public Clone
method. (Overriding Clone was already documented as deprecated in a
prior commit; this just adds the warning.)
Copy link
Contributor

@rpoyner-tri rpoyner-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, all commit messages.
Reviewable status: 2 unresolved discussions, needs at least two assigned reviewers

@jwnimmer-tri jwnimmer-tri force-pushed the pydrake-trajectory-clone branch from 2c7caf5 to 2f6169d Compare January 8, 2025 00:57
Copy link
Contributor

@rpoyner-tri rpoyner-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 r3, all commit messages.
Reviewable status: needs at least two assigned reviewers

Copy link
Collaborator Author

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

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

+@sherm1 for off-schedule platform review, please. Looking for some fresh eyes on this as the on-call people this week are already primed with a lot of background knowledge.

Reviewed 6 of 7 files at r1, 1 of 1 files at r3, all commit messages.
Reviewable status: LGTM missing from assignee sherm1(platform)


bindings/pydrake/trajectories_py.cc line 205 at r1 (raw file):

Previously, rpoyner-tri (Rick Poyner (rico)) wrote…

nit This comment doesn't quite do it for me. My first reading missed the crucial role of WrappedTrajectory in this particular caper.

Maybe instead: "Stuff the shared pointer inside a WrappedTrajectory, and return that via unique_ptr to meet our required return signature."

"caper" -- I love it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: low release notes: none This pull request should not be mentioned in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants