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

debug-only assertions in rotational inertia are hostile to uses in parsing #22415

Open
rpoyner-tri opened this issue Jan 7, 2025 · 3 comments
Assignees
Labels
component: multibody plant MultibodyPlant and supporting code type: bug

Comments

@rpoyner-tri
Copy link
Contributor

What happened?

Debug-only error checking behavior in RotationalInertia:

  • forces me to bifurcate tests on compilation mode
  • is generally hostile to the rule that errors in parsing flow to a diagnostic policy, rather than just throwing.

TBD: example of a split test, investigation into execution paths and alternatives.
Victory condition: parser unit tests don't need to use #ifdef to capture divergent behavior based on compilation mode.

Version

No response

What operating system are you using?

Ubuntu 22.04

What installation option are you using?

compiled from source code using Bazel

Relevant log output

No response

@rpoyner-tri rpoyner-tri self-assigned this Jan 7, 2025
@jwnimmer-tri
Copy link
Collaborator

jwnimmer-tri commented Jan 7, 2025

Yup, I've been pondering this, too.

The purpose of DRAKE_ASSERT is to identify bugs inside Drake (in our Debug CI jobs). Using DRAKE_ASSERT to sanitize data from users is wholly inappropriate.

(It is particularly relevant to parser, but even without any parsing involved -- having users call a RotationMatrix constructor that uses DRAKE_ASSERT for validating its arguments is wrong in the first place.)

@rpoyner-tri
Copy link
Contributor Author

Example of a split test case is in #22414 .

@jwnimmer-tri jwnimmer-tri added the component: multibody plant MultibodyPlant and supporting code label Jan 8, 2025
@rpoyner-tri
Copy link
Contributor Author

The problem seen in early revisions of #22414 was able to be avoided, but the potential for surprises remains. I'll need to study this a bit more to better reframe the problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: multibody plant MultibodyPlant and supporting code type: bug
Projects
None yet
Development

No branches or pull requests

2 participants