<p>Comments on <code>sdp.hpp</code>/<code>sdp.cpp</code>:</p>
<ul class="task-list">
<li><p>The basic architecture here is that <code>SDP</code> is an object representing the problem, which is then solved by <code>PrimalDualSolver</code>. I like this architecture, but we should probably adapt <code>LRSDP</code> to take <code>SDP</code> objects also, instead of holding all the information about the problem internally. This allows us, at the higher level, to provide interfaces like <code>LMNN<LRSDP></code> or <code>LMNN<PrimalDualSolver></code>.</p></li>
<li><p>Why provide <code>N2Bar()</code>? The <code>SDP</code> class doesn't actually use it.</p></li>
<li><p>I don't know that I agree with storing both a sparse and dense objective. I'd actually lean towards templatizing <code>SDP</code> so you get <code>SDP<ObjectiveMatrixType></code> 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.)</p></li>
<li><p>If you're highly against templatizing the objective matrix type, you can avoid holding <code>hasModifiedDenseObjective</code> and <code>hasModifiedSparseObjective</code> like this:</p></li>
</ul>
<pre><code>bool HasSparseObjective() const { return !(sparseC.n_elem == 0); }
bool HasDenseObjective() const { reutrn !(denseC.n_elem == 0); }
</code></pre>
<p>It's also worth pointing out that the current architecture allows the user to get into really weird situations if they call <code>SparseC()</code> then also <code>DenseC()</code> for some reason. The solution I just wrote doesn't help that situation at all.</p>
<p>Comments on <code>primal_dual.hpp</code>/<code>primal_dual.cpp</code>:</p>
<ul class="task-list">
<li><p>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.</p></li>
<li><p><code>Optimize()</code> returns a <code>std::pair<bool, double></code>. The <code>bool</code> is the success indicator and the <code>double</code> is the objective. Other optimizers just return the final objective, though. It seems like <code>false</code> 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 <code>bool</code> and a <code>double</code> as opposed to just the objective value.</p></li>
<li><p>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. <code>dualInfeas</code> instead of <code>dual_infeas</code>), 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.</p></li>
<li><p>I think templatizing <code>SDP</code> to have a sparse or dense objective function will condense the code in <code>Optimize()</code> significantly; no need for all those conditionals.</p></li>
<li><p>line 203: <code>rd = - sz - Asparse.t() * ...</code>: is that a typo? It's odd to see a negative sign separated from the argument like that, so I just wanted to check.</p></li>
<li><p>You can allow (basically) unlimited iterations by modifying line 191 like so:</p></li>
</ul>
<p><code>for (size_t iteration = 1; iteration != maxIterations; ++iteration)</code></p>
<p>and then a user can pass <code>maxIterations = 0</code> for no iteration limit.</p>
<ul class="task-list">
<li><p><code>DenseFromSparse()</code> seems overkill to me; you can just use <code>arma::mat()</code> instead. Also, it might be worth templatizing <code>Svec()</code> to work with sparse matrices. I'd be happy to do that templatization.</p></li>
<li><p>For the sake of consistency with other mlpack algorithms, I'd choose <code>initialX</code> as a parameter name, as opposed to <code>X0</code>.</p></li>
</ul>
<p>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. :)</p>
<p style="font-size:small;-webkit-text-size-adjust:none;color:#666;">—<br>Reply to this email directly or <a href="https://github.com/mlpack/mlpack/issues/387#issuecomment-70578268">view it on GitHub</a>.<img alt="" height="1" src="https://github.com/notifications/beacon/AJ4bFNgWyKzjcLMVoQjFOL-O7crD2if-ks5njYhDgaJpZM4DTlFe.gif" width="1" /></p>
<div itemscope itemtype="http://schema.org/EmailMessage">
<div itemprop="action" itemscope itemtype="http://schema.org/ViewAction">
<link itemprop="url" href="https://github.com/mlpack/mlpack/issues/387#issuecomment-70578268"></link>
<meta itemprop="name" content="View Issue"></meta>
</div>
<meta itemprop="description" content="View this Issue on GitHub"></meta>
</div>