add adaptive_rho, polish and maximum iteration options for OSQP #234

VenusPasandi · 2022-10-18T14:13:12Z

With this PR, we add some options to the osqp block, namely:

the flag for enabling and disabling the adaptive rho algorithm,
the flag for enabling and disabling the polish algorithm,
the maximum number of iterations

toolbox/library/src/OSQP.cpp

traversaro · 2022-10-18T14:20:03Z

toolbox/library/src/OSQP.cpp

+ if (maxIter > 0){
+ pImpl->maxIterations = maxIter;
+ } else {
+ bfError << "The maximum number of iterations is not valid.";


Probably it is helpful to either return false or explain that we are going to use the maxIter value of 1 ?

I think using the default value of 1 is a nice idea. Please see a3fa2f8

I agree with @traversaro that is better to make the block fail rather than applying something the user has not explicitly configured (example: I set -2 and magically the block sets 1. I'd prefer a failing simulation that tell me that I have to set an integeter > 0). The semantics of bfError is the scary red warning in the simulink GUI, and the model should fail in this case.

Furthemore, as replied in #234 (review), choosing a default value for the number of iterations that differs from the previous state (that was undefined, does OSQP has a documented number of iterations when not set?) would make this PR not backward compatible.

I suggest the following solutions:

If OSQP has a documented number of iterations that is selected when the user does not specify it (-> what we were doing before this PR), use this number as default

Othetwise, initialize in the mask an invalid number of iterations (e.g. -1) and do not call the setMaxIteration if it's less than 1. In this case, we can notify the user in the GUI with a message with low verbosity (lower than warning)

Actually, I think we don't need to check the validity of the maxIterations in the C++ library, because in the Simulink GUI,

I defined an initial value (which is equal to 100 but I think we need to set it to 4000 for backward compatibility),

I defined some constraints for the maximum iteration parameter, so in reality, the user can only choose a positive real number otherwise, the Simulink GUI generates an error.

So, I think we could remove this check from the C++ side.

traversaro · 2022-10-18T14:20:45Z

matlab/library/WBToolboxLibrary_repository.mdl

@@ -1,14 +1,14 @@
 Library {
- Name "WBToolboxLibrary_repository2"
+ Name "WBToolboxLibrary_repository"


@VenusPasandi did you renamed explicitly the library? Is that intentional?

I have no memory of this but the 2 could have been there for showing simultaneously the mdl (developement version with newer matlab version) and slx libraries (deployed version, compatible with old matlab version).

In view of this, please double check that the committed simulink libraries are exported with the correct simulink versions (that I don't remember where we specified it).

I have no idea about this extra "2". I didn't change the name of the library.

As I checked, previously it was "WBToolboxLibrary_repository", but then it changes to "WBToolboxLibrary_repository2" in the commit a1b8100.

In view of this, please double check that the committed simulink libraries are exported with the correct simulink versions (that I don't remember where we specified it).

Yes, it is correct. The exported library is in R2016b.

Then ok, I guess that removing the 2 is ok, no idea why in that PR it was changed

traversaro · 2022-10-18T14:24:36Z

Thanks @VenusPasandi ! It seems great. The only doubt is how to handle the backward compatibility. For this, I probably need the help of some Simulink expert (perhaps @gabrielenava @diegoferigo can be help me here). Now that we changed the mask of the block, what happens to models that were using the old OSQP block without this parameters? They will fail? They will use the default values of the new parameters? If they fail, what will be the error message?

traversaro · 2022-10-18T14:27:09Z

I do not know if https://it.mathworks.com/help/simulink/ug/make-backward-compatible-changes-to-libraries.html may be useful in this context.

diegoferigo

Added my review, nothing super important but nice to have. Approving in any case.

The only doubt is how to handle the backward compatibility.
[...]
Now that we changed the mask of the block, what happens to models that were using the old OSQP block without this parameters? They will fail? They will use the default values of the new parameters? If they fail, what will be the error message?

It's long time I don't develop on Matlab/Simulink and I don't remember exactly, but I think that as long as the simulink mask initializes these variables so that their values match the previous hardcoded numbers, the behaviour of existing simulink models should not be affected.

toolbox/library/include/WBToolbox/Block/OSQP.h

toolbox/library/src/OSQP.cpp

diegoferigo · 2022-10-18T14:35:28Z

toolbox/library/src/OSQP.cpp

@@ -338,6 +352,8 @@ bool wbt::block::OSQP::initialize(BlockInformation* blockInfo)
 // PARAMETERS
 // ==========

+ int maxIter = 1;


Maybe initializing to an invalid number would ensure that the parameter is correctly read from the mask

Suggested change

int maxIter = 1;

int maxIter = -1;

VenusPasandi · 2022-10-18T15:32:31Z

Now that we changed the mask of the block, what happens to models that were using the old OSQP block without these parameters? They will fail? They will use the default values of the new parameters? If they fail, what will be the error message?

@traversaro generally int he Simulink, there is no issue with backward compatibility unless,

we change the number of inputs/outputs of a block,
we add a parameter to the mask which is not initialized.

In this PR, we don't change the input/output of the block and we initialized the three parameters that we added to the mask. So, I think this PR is backward compatible.

VenusPasandi · 2022-10-18T16:10:11Z

I cleaned the PR and merged all the commits in one clean commit.

diegoferigo

another pass, lgtm

toolbox/library/src/OSQP.cpp

toolbox/library/include/WBToolbox/Block/OSQP.h

diegoferigo · 2022-10-18T16:54:11Z

Sorry @VenusPasandi, last thing. I was checking #231. Since we're updating this block and while waiting @traversaro's green flag, would you mind to replace the deprecated call with the new one?

gabrielenava · 2022-10-19T14:47:02Z

The only doubt is how to handle the backward compatibility. For this, I probably need the help of some Simulink expert (perhaps @gabrielenava @diegoferigo can be help me here). Now that we changed the mask of the block, what happens to models that were using the old OSQP block without this parameters? They will fail? They will use the default values of the new parameters? If they fail, what will be the error message?

It's long time I don't develop on Matlab/Simulink and I don't remember exactly, but I think that as long as the simulink mask initializes these variables so that their values match the previous hardcoded numbers, the behaviour of existing simulink models should not be affected.

@traversaro generally int he Simulink, there is no issue with backward compatibility unless,

we change the number of inputs/outputs of a block,
we add a parameter to the mask which is not initialized.
In this PR, we don't change the input/output of the block and we initialized the three parameters that we added to the mask. So, I think this PR is backward compatible.

Agree with both comments, I don't think there should be issues. In any case in IRonCub we use the OSQP block and WBTollbox in master, I can test it (even after merging the PR, so I will not block it) with the iRonCub controller and verify that our hypotheses are correct

traversaro · 2022-10-19T14:56:38Z

Ok, you seem quite confindent on Simulink robustness to adding parameters. Then, for me we can merge once you add a description on this change in https://github.com/robotology/wb-toolbox/blob/master/CHANGELOG.md @VenusPasandi .

VenusPasandi · 2022-10-20T08:18:27Z

Sorry @VenusPasandi, last thing. I was checking #231. Since we're updating this block and while waiting @traversaro's green flag, would you mind to replace the deprecated call with the new one?

It is done in f7fbf5b

VenusPasandi · 2022-10-20T08:18:56Z

Ok, you seem quite confindent on Simulink robustness to adding parameters. Then, for me we can merge once you add a description on this change in https://github.com/robotology/wb-toolbox/blob/master/CHANGELOG.md @VenusPasandi .

The changelog is updated with the new changes.

traversaro · 2022-10-20T08:20:27Z

Thanks!

VenusPasandi requested a review from traversaro as a code owner October 18, 2022 14:13

traversaro reviewed Oct 18, 2022

View reviewed changes

toolbox/library/src/OSQP.cpp Outdated Show resolved Hide resolved

traversaro reviewed Oct 18, 2022

View reviewed changes

toolbox/library/src/OSQP.cpp Show resolved Hide resolved

traversaro reviewed Oct 18, 2022

View reviewed changes

diegoferigo approved these changes Oct 18, 2022

View reviewed changes

add adaptive_rho, polish and maximum iteration options for OSQP

79ae06f

VenusPasandi force-pushed the master branch from 8a3cc86 to 79ae06f Compare October 18, 2022 16:08

diegoferigo approved these changes Oct 18, 2022

View reviewed changes

toolbox/library/src/OSQP.cpp Outdated Show resolved Hide resolved

toolbox/library/include/WBToolbox/Block/OSQP.h Outdated Show resolved Hide resolved

run clang-format

b31a26e

VenusPasandi added 2 commits October 20, 2022 09:53

replace the depricated solve() by solveProblem()

f7fbf5b

update change log

f8ca771

traversaro merged commit 385f047 into robotology:master Oct 20, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add adaptive_rho, polish and maximum iteration options for OSQP #234

add adaptive_rho, polish and maximum iteration options for OSQP #234

VenusPasandi commented Oct 18, 2022

traversaro Oct 18, 2022

VenusPasandi Oct 18, 2022

diegoferigo Oct 18, 2022

diegoferigo Oct 18, 2022

VenusPasandi Oct 18, 2022

traversaro Oct 18, 2022

diegoferigo Oct 18, 2022

VenusPasandi Oct 18, 2022

diegoferigo Oct 18, 2022

traversaro commented Oct 18, 2022

traversaro commented Oct 18, 2022 •

edited

Loading

diegoferigo left a comment

diegoferigo Oct 18, 2022

VenusPasandi commented Oct 18, 2022

VenusPasandi commented Oct 18, 2022

diegoferigo left a comment

diegoferigo commented Oct 18, 2022

gabrielenava commented Oct 19, 2022

traversaro commented Oct 19, 2022

VenusPasandi commented Oct 20, 2022

VenusPasandi commented Oct 20, 2022

traversaro commented Oct 20, 2022

add adaptive_rho, polish and maximum iteration options for OSQP #234

add adaptive_rho, polish and maximum iteration options for OSQP #234

Conversation

VenusPasandi commented Oct 18, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

traversaro commented Oct 18, 2022

traversaro commented Oct 18, 2022 • edited Loading

diegoferigo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VenusPasandi commented Oct 18, 2022

VenusPasandi commented Oct 18, 2022

diegoferigo left a comment

Choose a reason for hiding this comment

diegoferigo commented Oct 18, 2022

gabrielenava commented Oct 19, 2022

traversaro commented Oct 19, 2022

VenusPasandi commented Oct 20, 2022

VenusPasandi commented Oct 20, 2022

traversaro commented Oct 20, 2022

traversaro commented Oct 18, 2022 •

edited

Loading