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

[analysis] Update MonteCarlo to use Parallelism API #20785

Merged

Conversation

jwnimmer-tri
Copy link
Collaborator

@jwnimmer-tri jwnimmer-tri commented Jan 17, 2024

+@calderpg-tri for feature review, please.


This change is Reviewable

@jwnimmer-tri jwnimmer-tri added priority: low release notes: newly deprecated This pull request contains new deprecations release notes: fix This pull request contains fixes (no new features) labels Jan 17, 2024
Copy link
Contributor

@calderpg-tri calderpg-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 5 of 5 files at r1, all commit messages.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee calderpg-tri, needs platform reviewer assigned, needs at least two assigned reviewers


systems/analysis/test/monte_carlo_test.cc line 292 at r1 (raw file):

                       num_samples, &generator, kNoConcurrency);
  MonteCarloSimulation(make_simulator, &GetScalarOutput, final_time,
                       num_samples, &generator, kUseHardwareConcurrency);

It probably doesn't matter for such a simple test system, but note that this will use more threads than before (and more than are specified in the test rule).

The following are deprecated:
- drake::systems::analysis::kNoConcurrency
- drake::systems::analysis::kUseHardwareConcurrency
- drake::systems::analysis::MonteCarloSimulation overload that accepts
  the num_parallel_executions bare integer.
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 platform review per schedule, please.

Reviewable status: 1 unresolved discussion, LGTM missing from assignees calderpg-tri,sherm1(platform)


systems/analysis/test/monte_carlo_test.cc line 292 at r1 (raw file):

Previously, calderpg-tri wrote…

It probably doesn't matter for such a simple test system, but note that this will use more threads than before (and more than are specified in the test rule).

Good point, that was doing more harm than good.

Copy link
Contributor

@calderpg-tri calderpg-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 1 files at r2, all commit messages.
Reviewable status: LGTM missing from assignee sherm1(platform)

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.

Platform :lgtm: with one ignorable suggestion

Reviewed 4 of 5 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: 1 unresolved discussion


bindings/pydrake/systems/analysis_py.cc line 404 at r2 (raw file):

                          -> std::vector<RandomSimulationResult> {
          return MonteCarloSimulation(make_simulator, output, final_time,
              num_samples, generator, /* parallelism = */ Parallelism::None());

BTW the /* parallelism = */ comment seems superfluous here (would make sense if using the false abbreviation)

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.

Reviewable status: :shipit: complete! all discussions resolved, LGTM from assignees calderpg-tri,sherm1(platform)


bindings/pydrake/systems/analysis_py.cc line 404 at r2 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

BTW the /* parallelism = */ comment seems superfluous here (would make sense if using the false abbreviation)

Fair, but for my taste I like highlighting the pseudo-py::arg name here, as a kind of placeholder, grep target, etc. Omitting a kwarg in a binding is quite unusual, so the more I can do to help call attention to the oddity, the better (I hope).

@jwnimmer-tri jwnimmer-tri merged commit 9ec6c1d into RobotLocomotion:master Jan 17, 2024
10 checks passed
@jwnimmer-tri jwnimmer-tri deleted the parallelism-montecarlo branch January 17, 2024 22:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: low release notes: fix This pull request contains fixes (no new features) release notes: newly deprecated This pull request contains new deprecations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants