RobotLocomotion / drake

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

Add `solver_interface` and `solver_option` as optional members of `ConvexSet` #21251

Open AlexandreAmice opened 7 months ago

AlexandreAmice commented 7 months ago

Many of the method in ConvexSet rely on solving an optimization program to return an answer. Examples include IsEmpty(), PointInSet(), MaybeGetFeasiblePoint(), IsEmpty(), etc.

Sometimes, it would be good for the user to be able to both specify the solver used and potentially some options for that solver especially for debugging purposes.

This proposal is similar in scope to #20681 but much narrower.

AlexandreAmice commented 7 months ago

cc @hongkai-dai, @cohnt , @RussTedrake, and @sadraddini as all of you may have thoughts.

cohnt commented 7 months ago

+1 from me on this. I can think of multiple cases where I would leverage this!

AlexandreAmice commented 7 months ago

A draft PR is available at #21253 to discuss the design. An example of changing the method in HPolyhedron is also provided.

sadraddini commented 7 months ago

It is indeed useful. However, I feel that such an API is not right, as convex set object instances will carry extra information. Many convex set methods do not rely on optimization. I still prefer a more direct API such that for each method that solves a mathematical program, the user can specify the solver. Also, the user may want to use different solvers for different methods with the same ConvexSet object. That can be achieved with convex_set->SetSolverInterface(foo) or convex_set->SetSolverOptions(foo), but feels unnatural. Again, I agree that this eventually will be useful. My comments is only about style.

jwnimmer-tri commented 7 months ago

For the design with the solver as a shared_ptr member field of ConvexSet, it also raises difficult questions about thread safety. Previously, if I took a ConvexSet object and handed it off from one thread to another, the other thread could do as it pleases. If the solver is a shared_ptr member field, now we have possibly many sets on different threads all trying to call the same solver instance, which is not threadsafe.

On the other hand, the solver options can easily be either const or copied around, so do not raise questions about thread safety.

Still, aesthetically I agree with @sadraddini that having the options come in as arguments to one specific operation at a time is likely to be more sensible. If we could see some examples of how this would be used, that might sway the API design one way or another.

AlexandreAmice commented 7 months ago

Thank you all for commenting on the API, as I don't necessarily think my way is the best way. My high level goals with the current design was to avoid having to specify the solver instance every time a method is called. The other awkwardness I see with having the SolverInterface/SolverOption as an argument in each method at the ConvexSet level is that it may mistakenly give the impression that we do not provide efficient implementations for some operations.

The simplest example of this is the PointInSet method. Checking membership in a VPolytope will always require solving an LP and so at the ConvexSet interface level, we should allow specifying a SolverInterface and some SolverOptions. However, checking membership in an HPolytope is done without solving an optimization program i.e. just checking that Ax <= b is true and so it would be awkward to allow users to pass these spurious arguments.

Thoughts on how to resolve this?

That is a very good point about the shared pointer. @jwnimmer-tri I suppose copyable_unique_ptr may be the way to go with this design instead? I want to still be able to clone ConvexSet so I do need copy semantics .

jwnimmer-tri commented 2 months ago

... is that it may mistakenly give the impression that we do not provide efficient implementations for some operations.

I imagine we could solve that with documentation. The new solver arguments could explain that sometimes they will be ignored, in cases where there is an efficient closed-form solution and therefore no program will be solved in the first place.