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

Mujoco examples skip slow debug only #22416

Conversation

rpoyner-tri
Copy link
Contributor

@rpoyner-tri rpoyner-tri commented Jan 7, 2025

This change is Reviewable

Copy link
Contributor Author

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

+(status: do not merge) +(status: do not review)
Possible amendment to #22401 -- either as cherry-pick or subsequent patch.
FYI @RussTedrake @ggould-tri

Contains copies of #22401 and #22414 -- please dispose of those first.

Reviewable status: needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on @rpoyner-tri)

@rpoyner-tri rpoyner-tri force-pushed the mujoco-examples-skip-slow-debug-only branch from 22442d9 to 56e4198 Compare January 7, 2025 23:50
@rpoyner-tri rpoyner-tri added release notes: none This pull request should not be mentioned in the release notes and removed status: do not merge status: do not review labels Jan 7, 2025
Copy link
Contributor Author

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

#22401 is merged, and #22414 is indpendent of this one (and removed from this branch). This one can land at any time.
-(status: do not merge) -(status: do not review)
+@RussTedrake for feature review.
+(release notes: none)

Reviewable status: LGTM missing from assignee RussTedrake(platform), needs at least two assigned reviewers, labeled "do not merge" (waiting on @rpoyner-tri)

Copy link
Collaborator

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

Two nits below. (You can assign me for platform later if you wish.)

Reviewed 4 of 4 files at r2, all commit messages.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee RussTedrake(platform), needs at least two assigned reviewers


multibody/parsing/test/detail_mujoco_parser_examples_test.cc line 103 at r2 (raw file):

constexpr const std::string_view& DebugIsTooSlow(
    const std::string_view& non_debug_result) {

nit Pass string_view by value.

https://drake.mit.edu/styleguide/cppguide.html#Pass_By_Value_Primitives


multibody/parsing/test/detail_mujoco_parser_examples_test.cc line 104 at r2 (raw file):

constexpr const std::string_view& DebugIsTooSlow(
    const std::string_view& non_debug_result) {
#ifdef DRAKE_ASSERT_IS_ARMED

BTW if (kDrakeAssertIsArmed) { should work, and avoid preprocessor goo.

/// This header will define both `constexpr bool drake::kDrakeAssertIsArmed`
/// and `constexpr bool drake::kDrakeAssertIsDisarmed` globals.

Cases previously marked too slow will now run in non-debug builds, but are
still skipped in debug builds.
@rpoyner-tri rpoyner-tri force-pushed the mujoco-examples-skip-slow-debug-only branch from 56e4198 to 2b580a4 Compare January 8, 2025 00:28
Copy link
Contributor Author

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

Reviewable status: LGTM missing from assignee RussTedrake(platform), needs at least two assigned reviewers


multibody/parsing/test/detail_mujoco_parser_examples_test.cc line 103 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

nit Pass string_view by value.

https://drake.mit.edu/styleguide/cppguide.html#Pass_By_Value_Primitives

Sure, but how would I have known to treat string_view as a primitive?

Copy link
Contributor Author

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

+@jwnimmer-tri for platform review, eventually.

Reviewable status: LGTM missing from assignees jwnimmer-tri(platform),RussTedrake(platform)

Copy link
Collaborator

@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: LGTM missing from assignees jwnimmer-tri(platform),RussTedrake(platform)


multibody/parsing/test/detail_mujoco_parser_examples_test.cc line 103 at r2 (raw file):

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

Sure, but how would I have known to treat string_view as a primitive?

It would be worth updating the guide text to remove all doubt (ditto for std::span), but the ways you could have known are:

(1) Seeing it discussed in prior reviews.
(2) Recognizing that string_view is a simple pair of words (char* and size_t).

Copy link
Collaborator

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

:lgtm: platform (assuming it says basically intact after feature).

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

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: nice!

Reviewed 4 of 4 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: :shipit: complete! all discussions resolved, LGTM from assignees jwnimmer-tri(platform),RussTedrake(platform)

Copy link
Contributor

@ggould-tri ggould-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 4 of 4 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: :shipit: complete! all discussions resolved, LGTM from assignees jwnimmer-tri(platform),RussTedrake(platform)

@jwnimmer-tri jwnimmer-tri merged commit 7723bab into RobotLocomotion:master Jan 8, 2025
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

4 participants