Skip to content

[Small Feature] OSQP Support in Solver Interface#5741

Merged
lrineau merged 26 commits intoCGAL:masterfrom
danston:Solvers-add_osqp-danston
Jul 27, 2021
Merged

[Small Feature] OSQP Support in Solver Interface#5741
lrineau merged 26 commits intoCGAL:masterfrom
danston:Solvers-add_osqp-danston

Conversation

@danston
Copy link
Contributor

@danston danston commented May 31, 2021

This PR adds the OSQP solver support in CGAL. This solver is required in order to efficiently compute the convex Quadratic Programming (QP) problem, arising in the context of the new CGAL package called Shape Regularization (see #5742). This PR also better structures the Solver_interface package by splitting all solvers into several common categories. An additional example is added, too.

Tasks:

  • Check memory leaking.
  • Remove tmp modif in the docs.

Release Management

  • Affected package(s): Solver_interface
  • Issue(s) solved (if any): no issues
  • Feature/Small Feature (if any): Small Feature pre-approved on June 28, 2021
  • Link to compiled documentation (obligatory for small feature): docs
  • License and copyright ownership: no change

@danston danston marked this pull request as ready for review June 4, 2021 13:39
@maxGimeno maxGimeno added this to the 5.4-beta milestone Jun 4, 2021
danston and others added 2 commits June 8, 2021 16:24
- Fix wrong array sizes
- Fix to_CSC matrix conversion
- Rework the reserve() / insertion / size mechanisms
- Enable passing some options via solve()
@MaelRL MaelRL force-pushed the Solvers-add_osqp-danston branch from 8334712 to f99d0c3 Compare June 10, 2021 14:10
@danston danston changed the title OSQP Support in Solver Interface [Small Feature] OSQP Support in Solver Interface Jun 11, 2021
@danston danston added Not yet approved The feature or pull-request has not yet been approved. Pkg::Solver_interface Small feature labels Jun 11, 2021
@danston danston requested a review from afabri June 11, 2021 15:01
@afabri
Copy link
Member

afabri commented Jun 11, 2021

  • Instead of #if defined(CGAL_USE_OSQP) deal with it in the CMakeLists.txt
  • The brief documentation should start lower case
  • The sentence "It is also possible to derive new models from other quadratic programming libraries such as OOQP for example." is not really of interest.
  • "Note that you should .. " Use \note Also "should" makes no sense in a spec. Do you mean "must"? Or is it correct, but a waste of time?
  • "all existing/previous entries". Remove existing and previous and maybe even the entire sentence.
  • So this solver supports any FT ?
  • Does one use just the C OSQP, or the C++ wrapper?

@danston
Copy link
Contributor Author

danston commented Jun 14, 2021

@afabri Thank you for the review.

  • Instead of #if defined(CGAL_USE_OSQP) deal with it in the CMakeLists.txt - And what about other examples? The mixed-integer example in this package has been written this way that is it is dealing with libraries inside the cpp. I just followed that style. Should I change them as well?
    -- I fixed it both for the osqp and mixed_integer examples.

  • So this solver supports any FT ? - As input yes, internally everything is converted to double because the OSQP requires c_float types.

  • Does one use just the C OSQP, or the C++ wrapper? - It is the C version.

@afabri
Copy link
Member

afabri commented Jun 17, 2021

It is hard to see which review items are done and which not. Maybe copy and mark done as we do on the wiki.
Concerning the FT template parameter I am not sure that it makes sense if the package only works with double. @sloriot @lrineau your opinon?
@MaelRL had problems with underconstrained systems. Is that now fixed? Can it be underconstrained? If not then this merits maybe a \warning.
Is a c_float a float or a double?

@danston
Copy link
Contributor Author

danston commented Jun 18, 2021

Instead of #if defined(CGAL_USE_OSQP) deal with it in the CMakeLists.txt

  • Done. I fixed it both for the osqp and mixed_integer examples.

The brief documentation should start lower case

  • Done. I fixed all brief.

The sentence "It is also possible to derive new models from other quadratic programming libraries such as OOQP for example." is not really of interest.

  • Done. I removed it.

"Note that you should .. " Use \note Also "should" makes no sense in a spec. Do you mean "must"? Or is it correct, but a waste of time?

  • Done. I rephrased that.

"all existing/previous entries". Remove existing and previous and maybe even the entire sentence.

  • Done. I removed that.

So this solver supports any FT ?

  • The wrapper yes, but internally it is C float.

Does one use just the C OSQP, or the C++ wrapper?

  • It is the C version.

@danston
Copy link
Contributor Author

danston commented Jul 13, 2021

@lrineau lrineau self-assigned this Jul 16, 2021
@MaelRL MaelRL added Accepted small feature and removed pre-approved For pre-approved small features. After 15 days the feature will be accepted. labels Jul 20, 2021
@maxGimeno
Copy link
Contributor

@danston danston removed the request for review from afabri July 27, 2021 13:10
@lrineau lrineau added the rm only: ready for master For the release team only: that indicates that a PR is about to be merged in 'master' label Jul 27, 2021
@lrineau lrineau merged commit b9743ff into CGAL:master Jul 27, 2021
@lrineau lrineau removed Ready to be tested Under Testing rm only: ready for master For the release team only: that indicates that a PR is about to be merged in 'master' labels Jul 27, 2021
@danston danston deleted the Solvers-add_osqp-danston branch August 18, 2021 11:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants