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

Improve public API for user-facing types #577

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Conversation

imciner2
Copy link
Member

@imciner2 imciner2 commented Aug 4, 2023

We have three main user-facing types that are used to pass data into the solver: OSQPSettings, OSQPCscMatrix and OSQPCodegenDefines. Currently, our system requires the user to malloc the object, then do a separate call to get the defaults, then use them. This can be very awkward, and leads to having a lot of boilerplate code like

OSQPCscMatrix* P = malloc(sizeof(OSQPCscMatrix));
OSQPCscMatrix* A = malloc(sizeof(OSQPCscMatrix));

/* Populate matrices */
csc_set_data(A, m, n, A_nnz, A_x, A_i, A_p);
csc_set_data(P, n, n, P_nnz, P_x, P_i, P_p);

or

settings = (OSQPSettings *)malloc(sizeof(OSQPSettings));

osqp_set_default_settings(settings);
settings->polishing = 1;

With this PR, new functions to allocate and free these objects are added:

  • OSQPCscMatrix_new()
  • OSQPCscMatrix_free()
  • OSQPSettings_new()
  • OSQPSettings_free()
  • OSQPCodegenDefines_new()
  • OSQPCodegenDefines_free()

Additionally, the csc_set_data() function has been renamed to OSQPCscMatrix_set_data().

These changes mean that the boilerplate code from above becomes:

/* Create CSC matrices that are backed by the above data arrays. */
OSQPCscMatrix* P = OSQPCscMatrix_new(n, n, P_nnz, P_x, P_i, P_p);
OSQPCscMatrix* A = OSQPCscMatrix_new(m, n, A_nnz, A_x, A_i, A_p);

and

OSQPSettings* settings = OSQPSettings_new();
settings->polishing = 1;

These 6 new functions are only available in non-embedded mode since they allocate data. The OSQPCscMatrix_set_data() function is still available in all modes, since it will just assign pointers (so it is still a direct replacement for csc_set_data().

@imciner2 imciner2 added this to the osqp 1.0 milestone Aug 4, 2023
@coveralls
Copy link

coveralls commented Aug 4, 2023

Pull Request Test Coverage Report for Build 5764374642

Details

  • 18 of 35 (51.43%) changed or added relevant lines in 1 file are covered.
  • 43 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.07%) to 80.23%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/osqp_api.c 18 35 51.43%
Files with Coverage Reduction New Missed Lines %
src/osqp_api.c 43 87.27%
Totals Coverage Status
Change from base Build 5751349393: -0.07%
Covered Lines: 3214
Relevant Lines: 4006

💛 - Coveralls

@bstellato
Copy link
Collaborator

This looks great! I would recommend changing the main interfaces to use this slightly modified API so that everything is consistent. Let's at least create issues on the corresponding repos.

@goulart-paul
Copy link
Collaborator

Agree that this is a bit nicer, but with this:

/* Create CSC matrices that are backed by the above data arrays. */ OSQPCscMatrix* A = OSQPCscMatrix_new(m, n, A_nnz, A_x, A_i, A_p);

It needs to be really clear what happens when I call OSQPCscMatrix_free. Specifically, the call to the _free is (presumably?) only freeing the container and not any of the data arrays that were passed to _new. Otherwise you get issues with cases like the examples that use stack allocated arrays.

It would also be nice to provide something like OSQPCscMatrix_identity(n) or OSQPCdcMatrix_zeros(m,n), but then the behaviour of the _free is confusing because the user would be required to free internal arrays that they hadn't directly allocated.

We could maybe add some internal data ownership flag to the CscMatrix type that indicates whether it's free function should also free its arrays.

Base automatically changed from develop-beta1 to master April 4, 2024 19:27
Comment on lines +69 to +70
//settings->linsys_solver = OSQP_DIRECT_SOLVER;
//settings->linsys_solver = OSQP_INDIRECT_SOLVER;

Check notice

Code scanning / CodeQL

Commented-out code Note

This comment appears to contain commented-out code.
Comment on lines +41 to +42
//settings->linsys_solver = OSQP_DIRECT_SOLVER;
//settings->linsys_solver = OSQP_INDIRECT_SOLVER;

Check notice

Code scanning / CodeQL

Commented-out code Note

This comment appears to contain commented-out code.
src/osqp_api.c Dismissed Show dismissed Hide dismissed
src/osqp_api.c Dismissed Show dismissed Hide dismissed
src/osqp_api.c Dismissed Show dismissed Hide dismissed
@imciner2
Copy link
Member Author

I've gone ahead and tweaked the API so that we now track owning and not-owning the arrays inside a CSC matrix, and free them accordingly. This comes alongside adding some helper functions to create diagonal matrices, the identity matrix and a matrix of all zeros.

@coveralls
Copy link

coveralls commented Aug 13, 2024

Pull Request Test Coverage Report for Build 10377784078

Details

  • 80 of 85 (94.12%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.03%) to 91.846%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/osqp_api.c 80 85 94.12%
Totals Coverage Status
Change from base Build 10377474357: 0.03%
Covered Lines: 3751
Relevant Lines: 4084

💛 - Coveralls

@bstellato
Copy link
Collaborator

WIth the new changes it looks great to me! @goulart-paul what do you think? I saw you left quite a few comments

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants