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

Add solver_interface and solver_option as optional members of ConvexSet #21251

Open
AlexandreAmice opened this issue Apr 3, 2024 · 6 comments
Assignees
Labels
component: mathematical program Formulating and solving mathematical programs; our autodiff and symbolic libraries type: feature request

Comments

@AlexandreAmice
Copy link
Contributor

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
Copy link
Contributor Author

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

@AlexandreAmice AlexandreAmice added the component: mathematical program Formulating and solving mathematical programs; our autodiff and symbolic libraries label Apr 3, 2024
@AlexandreAmice AlexandreAmice self-assigned this Apr 3, 2024
@cohnt
Copy link
Contributor

cohnt commented Apr 3, 2024

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

@AlexandreAmice
Copy link
Contributor Author

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

@sadraddini
Copy link
Contributor

sadraddini commented Apr 3, 2024

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
Copy link
Collaborator

jwnimmer-tri commented Apr 4, 2024

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
Copy link
Contributor Author

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 .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: mathematical program Formulating and solving mathematical programs; our autodiff and symbolic libraries type: feature request
Projects
None yet
Development

No branches or pull requests

4 participants