osqp / osqp-matlab

Matlab interface for OSQP
https://osqp.org/
Apache License 2.0
42 stars 25 forks source link

included update_warm_start functionality #37

Open raoul-herzog opened 3 years ago

CLAassistant commented 3 years ago

CLA assistant check
All committers have signed the CLA.

raoul-herzog commented 3 years ago

dear Ian,

the emosqp interface does indeed not offer the same functionality compared to the "class based" Matlab interface of OSQP.

best regards,

Raoul

De : Ian McInerney @.> Envoyé : vendredi 28 mai 2021 21:27 À : oxfordcontrol/osqp-matlab @.> Cc : Herzog Raoul @.>; Author @.> Objet : Re: [oxfordcontrol/osqp-matlab] included update_warm_start functionality (#37)

@imciner2 commented on this pull request.


In codegen/files_to_generate/emosqp_mex.chttps://github.com/oxfordcontrol/osqp-matlab/pull/37#discussion_r641773563:

@@ -299,6 +299,34 @@ void mexFunction(int nlhs, mxArray plhs[], int nrhs, const mxArray prhs[])

     return;

 }

Actually, I may have gotten confused here because I didn't see this was the emosqp file, so this change probably isn't actually needed (although we need to document this now in https://github.com/oxfordcontrol/osqp/blob/master/docs/codegen/matlab.rst as well).

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHubhttps://github.com/oxfordcontrol/osqp-matlab/pull/37#discussion_r641773563, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ATZN2G7NBHDTNKWBN53YWG3TP7U7JANCNFSM43MXTL3Q.

imciner2 commented 3 years ago

Sorry about that confusion. My second comment on the if statement guarding the call to the actual OSQP update function still needs to be addressed though, since only checking for y non-empty seems to be incorrect. That function requires both x and y, so we should be making sure they both are non-empty for the call and throwing an error if either are empty I believe.

raoul-herzog commented 3 years ago

Hi Ian,

in principle a warm start with only x and without y is possible, but indeed the question is whether it makes sense ...

By the way, the same question applies to the if (!strcmp("update_bounds", cmd)) a couple of lines above. Does it make sense to provide only l without u ?

Do you plan a new release of osqp in the next future ?

best regards from Western Switzerland,

Raoul

De : Ian McInerney @.> Envoyé : dimanche 30 mai 2021 15:30 À : oxfordcontrol/osqp-matlab @.> Cc : Herzog Raoul @.>; Author @.> Objet : Re: [oxfordcontrol/osqp-matlab] included update_warm_start functionality (#37)

Sorry about that confusion. My second comment on the if statement guarding the call to the actual OSQP update function still needs to be addressed though, since only checking for y non-empty seems to be incorrect. That function requires both x and y, so we should be making sure they both are non-empty for the call and throwing an error if either are non-empty I believe.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHubhttps://github.com/oxfordcontrol/osqp-matlab/pull/37#issuecomment-851000671, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ATZN2G74II4Z7HTXT7HH7XLTQI4VZANCNFSM43MXTL3Q.

imciner2 commented 3 years ago

in principle a warm start with only x and without y is possible, but indeed the question is whether it makes sense ...

The function to call if you only want to warm start one of them is osqp_warm_start_x or osqp_warm_start_y - the function osqp_warm_start requires that both x and y be provided, so we should be checking that both x and y are non-empty vectors if we use that function.

gbanjac commented 3 years ago

We plan to remove osqp_warm_start_x, osqp_warm_start_y, as well as the function for updating lower and upper bounds separately. We have already done that in develop-1.0 branch of the main OSQP repository. See the commits below: https://github.com/oxfordcontrol/osqp/commit/fab90f8b138aa48c9be71c1d1ddbbe45267a1308 https://github.com/oxfordcontrol/osqp/commit/e49cf60b86f5762d02a61c14e859c052bc2c33c5

imciner2 commented 3 years ago

Basically what I am saying we should do is have code before the call to osqp_warm_start like:

if ( mxIsEmpty(x) || mxIsEmpty(y) ) {
    mexErrMsgTxt("Both x and y must be given");
}

because otherwise we can get a segfault/random data if one isn't specified.