Re: GSoC 2015: Optimization Package: Non-linear and constrained least squares lsqcurvefit, lsqlin, lsqnonlin

[Olaf Till]

On Thu, Mar 05, 2015 at 12:44:52PM -0800, AsmaA wrote:
> Hi Olaf,
> 
> I installed Octave from sources in a VMware running Linux Mint.
> 
> I started with qp/quadprog because it was the easiest to start with.
> 
> The subtle differences between 'qp' and 'quadprog' are in function arguments
> and constraints.
> 
> I wrote a MATLAB compatible wrapper for qp [1]. And a simple example with
> which I checked it [2].
> 
> Could you please check them?

A few issues (my knowledge about quadprog is only based on its
documentation, the qp I compair with is from Octave 3.8.2-rc2):

1. quadprog doesn't (?) allow incompletely specified equality
   constraints, the whole pair (Aeg, beq) must be given or none of
   it. qp currently tolerates only giving the matrix, setting equality
   constraints to empty, but that's not what quadprog does. So please
   reconsider your 'elseif (nargin == 5)' case (throw an error).

2. qp currently disallows empty general inequality constraints. This
   does not hurt too much for qp since they are the last arguments
   before the optional options structure and so need not be given in
   case they are empty. But it does not work to simply map the
   argument positions of inequality constraints from quadprog to qp:

octave:10> quadprog (1, 0, [], [], 1, 1)
warning: quadprog uses Octaves qp function
error: qp: inequality constraint matrix has incorrect column dimension
error: called from:
error:   /usr/nfs/share/octave/3.8.2-rc2/m/optimization/qp.m at line 254, 
column 9
error:   /home/olaf/devel/octave-forge/mercurial/optim/quadprog.m at line 64, 
column 22

   You must not give these arguments to qp if they are empty.

3. You haven't made the returned values compatible to quadprog,
   i.e. 'exitflag' is missing and at least INFO.solveiter should be
   mapped to INFO.iterations.

Please take a look at some code in core Octave for Octaves coding
style. In particular:

1. space before opening paranthesis ('function_name ()', 'if ()'),
   except in indices (vector(1:2)).

2. always space after ',', except at line end

3. exactly one space before and after '=' and comparison operators

A few additional suggestions:

1. It's usual to use 'print_usage' instead of explicitly complain
   about incorrect number of arguments, e.g.:

  if ((nargs = nargin ()) < 2 || nargs > 10 || nargs == 5)
    print_usage ();
  endif

2. You could probably process varargin similar to the following way:

  function varargout = quadprog (varargin)

    ...

    nargs = nargin ();

    in_args = cat (2, varargin, cell (1, 11 - nargs)); # one argument
               # more than quadprog has, this is for unused ALB of qp

    ## do the argument mapping
    qp_args = in_args([9, 1, 2, 5, 6, 7, 8, 11, 3, 4, 10]);

    ## remove inequality constraint arguments if empty
    if (isempty (qp_args{10}))
      qp_args([8:10]) = [];
    endif

    ## only one line of code for qp call 
    [...] = qp (qp_args{:});

3. In setting output arguments, if you check
   if (nargout >= 2)
     ...
   endif
   if (nargout >= 3)
     ...
   endif
   # and so on ...

   you needn't repeat the code for output arguments already set.

4. It's better to call qp only with the requested number of output
   arguments, to give it the chance to spare some computations:

   outargs = cell (1, nargout ());

   [outargs{:}] = qp (...);

Olaf

-- 
public key id EAFE0591, e.g. on x-hkp://pool.sks-keyservers.net

signature.asc
Description: Digital signature