Python wrapper increased optional parameters #56
Conversation
| @@ -142,7 +201,7 @@ static PyObject *csolve(PyObject* self, PyObject *args, PyObject *kwargs) | |||
| PyArrayObject *Ai = NULL; | |||
| PyArrayObject *Ap = NULL; | |||
| PyArrayObject *b = NULL; | |||
| PyObject *dims, *verbose = NULL; | |||
| PyObject *dims, *verbose, *opts = NULL; | |||
There was a problem hiding this comment.
I'm wondering if we've made a silly mistake here: should this read
PyObject *dims = NULL;
PyObject *verbose = NULL;
PyObject *opts = NULL;
Better safe than sorry. :)
|
Except for my comment, it looks good to me. @adomahidi, any thoughts? |
|
I guess having the verbose still as an argument is for backward compatibility? Could we handle this internally and rather have only an "opts" argument? |
|
Yeah, we can; I left the choice to Thomas. If you have an opinion, though, On Mon, May 19, 2014 at 3:37 PM, Alexander Domahidi <
|
|
But maybe we don't have to break backwards compatibility - of that argument is not a dictionary then it's the old behavior, otherwise it's an opt dictionary. Would that work conceptually? On May 19, 2014, at 3:58 PM, Eric Chu notifications@github.com wrote:
|
|
Well, the argument would have to be named "verbose" which is really strange This means the last argument is "named". So you can call What you're proposing is something like or but the following would also work and would not. There's no good way to modify the prototype so that it works On Mon, May 19, 2014 at 4:00 PM, Alexander Domahidi <
|
|
I understand, thanks for the explanation. The my opinion is to break backwards compatibility, since it's cleaner, and the changes required in CVXPY and QCML are minimal. What do you think? On May 19, 2014, at 4:06 PM, Eric Chu notifications@github.com wrote:
|
|
up to thomas. :) On Mon, May 19, 2014 at 4:10 PM, Alexander Domahidi <
|
|
I don't have strong feelings on the matter, and we can easily roll verbose Thomas On Mon, May 19, 2014 at 4:12 PM, Eric Chu notifications@github.com wrote:
|
|
Dont worry about QCML. We can make the update and if something breaks, it's I think CVXPY privileges verbose because it had to be compatible with ECOS On Monday, May 19, 2014, ThomasLipp notifications@github.com wrote:
|
|
VERBOSE is now an argument to opts and has been removed as an argument breaking backwards compatibility. Added a bool parser. For some reason SCS is printing error messages in python to stdout. I switched the printing of error messages to stderr in ECOS, but if there's a good reason to have them print to stdout, we can switch that back. As currently implemented in cvxpy (will submit a pull request for that shortly), the VERBOSE option added as an optional argument takes precedence over the verbose entered as a keyword. I don't think there's really a way to do a workaround for this, since I'm not aware of a way to distinguish between a verbose=False argument and the default argument in python, so I think we just declare that a feature. If the verbose keyword takes precedents then "VERBOSE":True will have no effect since verbose will default to false and take precedence. |
|
Looks good to me. We should be printing to stderr, so that's a good fix. I think SCS prints to stdout, because I was doing it here. I think we're finally used enough for error reporting to matter. @adomahidi, thoughts? I'm not sure I understand your comment about the verbose option in CVXPY. Shouldn't the verbose keyword in CVXPY just be put into the opts structure now? |
|
So you still have the option to call cvxpy with p.solve(cp.ECOS,verbose=True) or p.solve(cp.ECOS,solver_specific_opts={'VERBOSE':True}). If you call p.solve(cp.ECOS,verbose=True,solver_specific_opts={'VERBOSE':False}), verbose will be set to false. This is the same behavior that SCS has. Steven wanted to keep verbose as a keywords as he viewed it more as an argument to cvxpy than as an argument to the solver. If we want to convince Stephen to eliminate the verbose argument that would of course eliminate this problem, but it may be more important to preserve cvxpy backwards compatibility. |
|
Sure, but nothing's stopping him from making it "work out" under the hood. On Wednesday, May 21, 2014, ThomasLipp notifications@github.com wrote:
|
|
I think adding *_kwargs to the ecos.solve method would be more pythonic than having an opts dictionary. At a minimum, I'd like to use *_kwargs to replace solver_specific_opts and verbose. It would be easier for me if the ecos interface worked the same way. It's less awkward to write than |
|
Hmmm. Thomas, how much work do u think it would be to modify your PR. I But make the keywords lowercase instead of uppercase now. There are ways around this from the cvxpy side, also. You can always just If it's a bit of work, I can fix this after we accept the PR. On Wednesday, May 21, 2014, Steven Diamond notifications@github.com wrote:
|
|
Probably won't be very hard, but I won't have any time to look at this until Friday at the earliest. |
|
Sure. If the PR isn't closed by then, then I haven't started working on it. On Wednesday, May 21, 2014, ThomasLipp notifications@github.com wrote:
|
|
So I'm still relatively new to python and kwargs, but this appears to work for me, but I'm uncertain on how to document it, and someone should check to make sure I haven't done anything foolish. |
|
I must admit I's also prefer Steven's suggestion - it's quite a nice way of setting only those parameters that you actually want to set. No need to deal with the whole dictionary. On May 21, 2014, at 7:20 PM, ThomasLipp notifications@github.com wrote:
|
|
I'm merging this PR so I can start working on some bug fixes for the 1.0.4 release candidate. |
Python wrapper increased optional parameters
Added options for tolerance variables and max iterations to the python wrapper for ecos offering the same functionality as MATLAB. I followed the SCS template where possible, so everything is handled almost exactly the same, except that I left verbose as its own argument to preserve backwards compatibility. I also followed the SCS model of providing limited information about the arguments in the readme.
I opted to statically allocate the settings array. this creates slightly more overhead (several floats) in the scenario where there are no opt arguments, but avoids needing to worry about memory allocation. If we're worried about the couple floats, we can switch to dynamic allocation.