-
Notifications
You must be signed in to change notification settings - Fork 11
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
Fast panoc implementation #5
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for this! I would really like to try it out, but as I mentioned I'm having problems mex
ing stuff on macos: this is one of a major turn offs for me with MATLAB (one of the others being the lack of free CI solutions for open source projects...)
@@ -105,27 +105,33 @@ | |||
if nargin < 4, aff = []; end | |||
if nargin < 5, constr = []; end | |||
if nargin < 6, opt = []; end | |||
|
|||
if(strcmp(opt.solver,'panoc')) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In principle zerofpr2
should be renamed panoc
at some point... (See #1, although I'm not sure if and when this reorganization will be finalized). Just to avoid conflicts I would try to use here strings for the solver selection that make clear that one is selecting an "external" one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that it should be clear that it's a external one. It is also the reason why i put it in a separate folder. It leads to an other question, should we even include the library inside ForBes or should we put it as a separate project and put it as optional dependency. And just leave the Matlab code that interfaces the two in place?
I don't like mex files either, nmpc-codegen doesn't use mex files but calllib and Cmake. But because i needed callback's here (c-code call's Matlab functions) i had to do it with a mex file.
I have an older mac at home, i will install Matlab 2016a on it and compile it. Considering compiled Mex files are forward (NOT backward) compatible you should be able to run it. I think binary's are probably the best way to go, it avoids users having to compile it themselves.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question. Maybe it's a good idea to keep this in a separate, and maybe provide it with its own MATLAB interface. The purpose of ForBES was that of having algorithms and functions and use them together, if a method implementation works with its own function library and options, then its probably cleaner to think about a MATLAB interface of its own...
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It already has its own Matlab interface (the mex file) i might add a slightly more friendly interface later on. The MATLAB code added in forbes here allows the usage of all the forbes constraint functions. The nice thing about this is, that you can switch between a C or MATLAB implementation by only changing the name of the solver.
Or maybe we can add a option "speedy" instead, that is by default false, but if true, forbes uses the C implementation if it is available. (so no overlap with zerofpr2)
So i would suggest: leave the clue between the library's in place: https://github.com/Zilleplus/ForBES/blob/master/external_library/src_Matlab/solve_panoc.m
But we don't put these binary files in here : https://github.com/Zilleplus/ForBES/tree/master/external_library/forbes_panoc/bin
We could call it CPanoc or so and add it in a repo on github. If you want to use a speedy library, you will need to install it. Right now its here: https://github.com/Zilleplus/draf_panoc
It's very doable to add a zerofpr.c to my library and with a little bit of code, you have the zerofpr and panoc solver in C. In that case the name CPanoc makes no sense.
Added a C implementation of panoc. Right now only the windows binary available. If you want to try it out with Linux or Mac run the install script in repo: https://github.com/Zilleplus/draf_panoc it will compile everything and put the compiled mex file in the Matlab folder.
Also only the C-implemenation of the box-constraint is available, in all other cases it will use the Matlab implemented constraint.
The install script in ForBes (setup.m) is separated as its more or less experimental right now.