RobotLocomotion / drake

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

AddL2NormCost is a footgun #20552

Closed AlexandreAmice closed 6 months ago

AlexandreAmice commented 11 months ago

Is your feature request related to a problem? Please describe. Despite the warning attached to the docs, I have seen several people mistakenly assume that AddL2NormCost will add a convex cost and then not realize that Drake will attempt to solve the program using a generic non-linear solver.

From an optimization standpoint, I cannot think of a time when the 2-norm squared is not an equally good substitute to the 2-norm.

Describe the solution you'd like Deprecated AddL2NormCost and replace it with AddL2NormSquaredCost which would simply be a wrapper to the currently implemented AddL2NormCostUsingConicConstraint.

@RussTedrake for discussion based on experience teaching Underactuated and Manipulation @hongkai-dai for thoughts.

EricCousineau-TRI commented 11 months ago

Per Platform checklist, assigning Hongkai.

RussTedrake commented 11 months ago

Quick thoughts:

So I definitely disagree with

From an optimization standpoint, I cannot think of a time when the 2-norm squared is not an equally good substitute to the 2-norm.

but I agree that it can be a misleading interface for users.

When I used it in KinematicTrajectoryOptimization, I did this

  /** Adds a cost on an upper bound on length of the path, ∫₀ᵀ |q̇(t)|₂ dt, by
  summing the distance between the path control points. If `use_conic_constraint
  = false`, then costs are added via MathematicalProgram::AddL2NormCost;
  otherwise they are added via
  MathematicalProgram::AddL2NormCostUsingConicConstraint. */
  void AddPathLengthCost(double weight = 1.0,
                         bool use_conic_constraint = false);

would having a single entry point with the argument help to force people to think about the consequences?

AlexandreAmice commented 11 months ago

Perhaps we could simply add an optional argument AddL2NormCost(A, b,vars, as_convex=False)? If the as_convex argument gets passed, then we call Add2NormSquaredCost instead.

There is also a TODO in the code to make the convex solvers directly accept the L2 norm cost.

RussTedrake commented 11 months ago

That was almost my proposal. But definitely don't add the squared cost if someone asked for the 2norm cost.

hongkai-dai commented 6 months ago

I am working on a sequence of PRs, to support L2NormCost natively as convex costs in different solvers (SCS, Clarabel, Gurobi, Mosek). This would take a while to add this feature.

hongkai-dai commented 6 months ago

Now that the commonly-used convex solvers (Clarabel, SCS, Gurobi and Mosek) all support L2NormCost. I will close this issue.