RobotLocomotion / drake

Model-based design and verification for robotics.
https://drake.mit.edu
Other
3.16k stars 1.24k forks source link

QuadraticCost Does Not Check Matrix Shape #21578

Closed cohnt closed 1 week ago

cohnt commented 2 weeks ago

What happened?

When constructing a quadratic cost, there are restrictions on the size of the input matrices Q and b. But these aren't being checked, so if a user specifies inputs of the wrong size, no error is thrown. Instead, uninitialized values will be used in the construction of the cost or constraint, leading to cryptic downstream errors.

Here's a simple example in python, demonstrating that the matrix shape is not being checked:

import numpy as np
from pydrake.all import QuadraticCost

QuadraticCost(np.eye(3), np.zeros(2), 1)

Version

b37aa6998b

What operating system are you using?

No response

What installation option are you using?

No response

Relevant log output

No response

hongkai-dai commented 2 weeks ago

The size is checked in the debug mode https://github.com/RobotLocomotion/drake/blob/b37aa6998b9488a7e64e97e37e67d2f7df8710bf/solvers/cost.h#L135-L136 Since pybind is built in the release mode, it bypasses the check.

@jwnimmer-tri I can activate the check in the release mode. But what would be our general strategy? It is quite hard for python users to switch to debug mode, so shall we prefer DRAKE_DEMAND to DRAKE_ASSERT in our C++ code?

jwnimmer-tri commented 2 weeks ago

See https://github.com/RobotLocomotion/drake/issues/7827. The functions DRAKE_DEMAND and DRAKE_ASSERT are only appropriate for Drake checking its own internal invariants. User input should always be checked with DRAKE_THROW_UNLESS (or something similar, like an if-test with a throw statement).

Ideally, any patches to this function would also change it obey https://drake.mit.edu/styleguide/cppguide.html#Inline_Functions, but if this an emergency maybe the platform reviewer will let you get away without doing that part.