Skip to content

[pydrake] Align with nanobind factory constructor API (part 3)#24751

Open
jwnimmer-tri wants to merge 1 commit into
RobotLocomotion:masterfrom
jwnimmer-tri:nanobind-simulator-init
Open

[pydrake] Align with nanobind factory constructor API (part 3)#24751
jwnimmer-tri wants to merge 1 commit into
RobotLocomotion:masterfrom
jwnimmer-tri:nanobind-simulator-init

Conversation

@jwnimmer-tri

@jwnimmer-tri jwnimmer-tri commented Jul 18, 2026

Copy link
Copy Markdown
Collaborator

Towards #21572.

Same as #24600 and #24742 -- we weren't sure at the time if these were also the right fixes, but it seems like they are.


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 Jul 18, 2026
@jwnimmer-tri

Copy link
Copy Markdown
Collaborator Author

@drake-jenkins-bot linux-noble-clang-bazel-experimental-undefined-behavior-sanitizer please

@jwnimmer-tri jwnimmer-tri left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

+a:@rpoyner-tri for co-feature review, please. I took this commit of yours from the branch and prepped it for master (reformatting).

:lgtm: co-feature review.

@jwnimmer-tri reviewed 6 files and all commit messages, and made 2 comments.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee rpoyner-tri(platform), needs at least two assigned reviewers (waiting on jwnimmer-tri and rpoyner-tri).


a discussion (no related file):
Working

Needs platform reviewer assigned.

@rpoyner-tri rpoyner-tri left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

:lgtm:

@rpoyner-tri reviewed 6 files and all commit messages, and made 2 comments.
Reviewable status: 2 unresolved discussions, needs at least two assigned reviewers (waiting on jwnimmer-tri).


systems/analysis/test/simulator_test.cc line 818 at r1 (raw file):

}

// Tests the internal use (for Python) in-place construction factory method

nit I now find this sentence hard to parse; lots of modifiers are piled up.

Two lame suggestions; perhaps you'll think of a better one.

Tests the (internal use for Python) in-place construction factory method
Tests the internal-use (for Python) in-place construction factory method

@rpoyner-tri rpoyner-tri left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@rpoyner-tri made 1 comment.
Reviewable status: 2 unresolved discussions, needs at least two assigned reviewers (waiting on jwnimmer-tri).


systems/analysis/test/simulator_test.cc line 818 at r1 (raw file):

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

nit I now find this sentence hard to parse; lots of modifiers are piled up.

Two lame suggestions; perhaps you'll think of a better one.

Tests the (internal use for Python) in-place construction factory method
Tests the internal-use (for Python) in-place construction factory method

Or perhaps even better:

Tests the in-place construction factory method

@jwnimmer-tri
jwnimmer-tri force-pushed the nanobind-simulator-init branch from b36bc26 to 2cca9f2 Compare July 19, 2026 16:26
@jwnimmer-tri jwnimmer-tri changed the title [pydrake] Align Simulator with nanobind factory constructor API [pydrake] Align with nanobind factory constructor API (part 3) Jul 19, 2026

@jwnimmer-tri jwnimmer-tri left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@jwnimmer-tri reviewed 2 files and all commit messages, made 2 comments, and resolved 2 discussions.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee ggould-tri(platform) (waiting on ggould-tri and rpoyner-tri).


a discussion (no related file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Working

Needs platform reviewer assigned.

+a:@ggould-tri for platform review per schedule, please.


a discussion (no related file):
@rpoyner-tri I found one more patch to cherry-pick. PTAL.

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