[mlpack-git] [mlpack] Add a primal-dual interior point SDP solver (#387)

[Ryan Curtin]

Comments on `sdp.hpp`/`sdp.cpp`:

* The basic architecture here is that `SDP` is an object representing the problem, which is then solved by `PrimalDualSolver`.  I like this architecture, but we should probably adapt `LRSDP` to take `SDP` objects also, instead of holding all the information about the problem internally.  This allows us, at  the higher level, to provide interfaces like `LMNN<LRSDP>` or `LMNN<PrimalDualSolver>`.

* Why provide `N2Bar()`?  The `SDP` class doesn't actually use it.

* I don't know that I agree with storing both a sparse and dense objective.  I'd actually lean towards templatizing `SDP` so you get `SDP<ObjectiveMatrixType>` or something.  This will accelerate code in the solver because you won't have to check if there is a sparse or dense objective.  (A smart compiler could actually figure that out at compile time fairly easily, but I'm not sure that gcc actually will.)

* If you're highly against templatizing the objective matrix type, you can avoid holding `hasModifiedDenseObjective` and `hasModifiedSparseObjective` like this:

```
bool HasSparseObjective() const { return !(sparseC.n_elem == 0); }
bool HasDenseObjective() const { reutrn !(denseC.n_elem == 0); }
```

It's also worth pointing out that the current architecture allows the user to get into really weird situations if they call `SparseC()` then also `DenseC()` for some reason.  The solution I just wrote doesn't help that situation at all.

Comments on `primal_dual.hpp`/`primal_dual.cpp`:

* I don't know what a lot of the parameters to the optimizer are, but I'm assuming you're planning to add some comments to the definitions of those parameters at some point.

* `Optimize()` returns a `std::pair<bool, double>`.  The `bool` is the success indicator and the `double` is the objective.  Other optimizers just return the final objective, though.  It seems like `false` is only returned when the solver does not converge in the specified number of iterations.  I'm not sure any higher-level code will ever use that information, though, so I don't know how useful it is to return both a `bool` and a `double` as opposed to just the objective value.

* Ok, this is hopefully the most pedantic thing I write, and I don't mean to be moving the goalposts or anything, but it looks like this was never written in the style guide (I'll update it in a moment).  Variable naming should preferably avoid underscores and use camelcase instead (i.e. `dualInfeas` instead of `dual_infeas`), for the sake of consistency.  I can change these after we merge it in; I don't like forcing trivial things like this onto other people's contributions, but I would like the code in master to eventually look like that.

* I think templatizing `SDP` to have a sparse or dense objective function will condense the code in `Optimize()` significantly; no need for all those conditionals.

* line 203: `rd = - sz - Asparse.t() * ...`: is that a typo?  It's odd to see a negative sign separated from the argument like that, so I just wanted to check.

* You can allow (basically) unlimited iterations by modifying line 191 like so:

```for (size_t iteration = 1; iteration != maxIterations; ++iteration)```

and then a user can pass `maxIterations = 0` for no iteration limit.

* `DenseFromSparse()` seems overkill to me; you can just use `arma::mat()` instead.  Also, it might be worth templatizing `Svec()` to work with sparse matrices.  I'd be happy to do that templatization.

* For the sake of consistency with other mlpack algorithms, I'd choose `initialX` as a parameter name, as opposed to `X0`.

I haven't sat down yet to really go through the Armadillo expressions and see if I can accelerate them, but I'll do that once you take a look through my comments and decide what should change (if anything).  As always, if I write something you disagree with, feel free to start a discussion.  I think more robust discussion leads to more robust abstractions. :)

---
Reply to this email directly or view it on GitHub:
https://github.com/mlpack/mlpack/issues/387#issuecomment-70578268
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mailman.cc.gatech.edu/pipermail/mlpack-git/attachments/20150119/8512f534/attachment.html>