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

Cleanup and modernize the wrapper #52

Merged
merged 13 commits into from
Dec 4, 2023
Merged

Cleanup and modernize the wrapper #52

merged 13 commits into from
Dec 4, 2023

Conversation

imciner2
Copy link
Member

@imciner2 imciner2 commented Nov 17, 2023

Various modernizations and cleanups to the OSQP mex function and wrapper class.

  • Improve readability and maintainability of the Matlab class by breaking functions into separate files
  • Simplify code sections for getting OSQP constants
  • Expose rho update in a nicer way
  • Introduce a better and more automated wrapper for the various structs used by OSQP
  • Introduce wrappers for copying/cloning vectors cleanly

@imciner2 imciner2 marked this pull request as ready for review December 1, 2023 14:04
Copy link
Collaborator

@AmitSolomonPrinceton AmitSolomonPrinceton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really like the changes and the new OSQP class, it's much easier to read and cleaner than what we previously had.
The only confusing thing in my opinion is that the installation instructions in the readme file are not updated.

parse(p, target_dir, varargin{:});

% Set internal variables
if strcmp(p.Results.parameters, 'vectors')
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe use Matlab enumeration instead.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This entire file needs to be rewritten to support the newer codegen paradigm anyway, so I didn't actually change anything in there. We should do this when we update that file though.

@osqp/setup.m Outdated Show resolved Hide resolved
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if it's worth the trouble, but I think having a function that checks if the input is not empty (the !in || numel==0 check) will make the code slightly more readable and easier to maintain. Please ignore this comment if you disagree.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there are only two places we ever really check for this, and they are both in this file, so I don't think a helper function makes as large a difference.

@imciner2
Copy link
Member Author

imciner2 commented Dec 4, 2023

The only confusing thing in my opinion is that the installation instructions in the readme file are not updated.

Are you referring to the ones linked to from the readme that are hosted online? We need to update those in the main OSQP repo to fix that once this is all merged.

@AmitSolomonPrinceton
Copy link
Collaborator

The only confusing thing in my opinion is that the installation instructions in the readme file are not updated.

Are you referring to the ones linked to from the readme that are hosted online? We need to update those in the main OSQP repo to fix that once this is all merged.

Both it and the instructions in the package directory.

@imciner2 imciner2 merged commit 391fe8f into develop-1.0 Dec 4, 2023
10 checks passed
@imciner2 imciner2 deleted the im/restructure branch December 4, 2023 23:21
@imciner2
Copy link
Member Author

imciner2 commented Dec 4, 2023

Both it and the instructions in the package directory.

Yea, the packaging script needs a rework to do a package builder to make distribution easier.

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.

2 participants