RobotLocomotion / drake

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

Fix bugs in Mosek and OSQP parsing of Quadratic Costs #21580

Closed AlexandreAmice closed 4 days ago

AlexandreAmice commented 1 week ago

What happened?

There appear to be bugs related to how the lower/upper triangular part of symmetric quadratic costs (and maybe constraints) are parsed by Mosek and OSQP. This is manifests as errors such as

MOSEK error 1415 (MSK_RES_ERR_QOBJ_UPPER_TRIANGLE): Only elements in the lower triangle of the quadratic term in the objective should be specified. The element q[0,3] is in the upper triangle.

and

ERROR in validate_data: P is not upper triangular
ERROR in osqp_setup: Problem data validation.

I have tracked down two of the possible places this is happening and opened #21579 to show where this is happening, but do not have time in the near future to write good tests.

Given the severity of this bug and the length of time these bugs have been open, I think it is worth reviewing all the quadratic parsing code and ensuring that we are getting what we expect.

Version

No response

What operating system are you using?

No response

What installation option are you using?

No response

Relevant log output

No response

AlexandreAmice commented 1 week ago

cc @hongkai-dai and @jwnimmer-tri.

rpoyner-tri commented 1 week ago

Since @hongkai-dai is away, assigning to @jwnimmer-tri for further disposition.

jwnimmer-tri commented 1 week ago

Beyond the already-existing PR, the remaining action is: "I think it is worth reviewing all the quadratic parsing code and ensuring that we are getting what we expect."

That is a fine call to action, but is no real hurry, so we can park the issue with the component owner +@hongkai-dai indefinitely (-@jwnimmer-tri).

jwnimmer-tri commented 1 week ago

Beyond the current PR's hotfix, ideally we have some programmatic way to never make this bug again. For example, if we used SortedPair<int> to denote the triangular indices, then we wouldn't need to remember to sort them (min/max) at every point of use. Or we could have class LowerTriangularRowCol that did something similar -- so we don't need to remember that .second() is row and .first() is col. Or Eigen has built-in triangular views that might help. Or we could have a helper function that operated on a vector<Eigen::Triplet<...>> and sorted the indices into lower triangular form post-facto. Etc.