<p>I've gone back and forth on this many times. In this particular case it seems like the fastest code would result by a <code>void SetRatio(double ratioValue)</code> function instead of a <code>double& Ratio()</code> function, since some post-processing of the value needs to be done. I'd suggest the function name <code>void Ratio(double ratio)</code> instead of <code>SetRatio()</code>, though, to be more consistent with the other mlpack code that has functions like this (I think this is also done in <code>GaussianKernel</code> and a few of the other kernels).</p>
<p>It's usually a subjective call to decide whether to go with a <code>void Parameter(type parameter)</code> or a <code>parameter& Parameter()</code> function, and I don't think there's a one-size-fits-all solution. For instance, working with matrices means that something like <code>void Matrix(mat& matrix)</code> will be very slow. Now that we use C++11, we can write <code>void Matrix(mat&& matrix)</code>, but this still can make expressions very awkward; consider adding 1 to the diagonals of the matrix. That'll give something like... <code>Matrix(std::move(Matrix()) + arma::eye<arma::mat>(Matrix().n_rows, Matrix().n_cols)))</code>, but if you were able to work directly on the matrix you could just write <code>Matrix() += arma::eye<arma::mat>(Matrix().n_rows, Matrix().n_cols)</code>.</p>
<p>I think that in this case, moving the calculation of <code>scale</code> outside of <code>Forward()</code> may not actually give any noticeable runtime improvement, since the vast majority of the work spent in <code>Forward()</code> will be in the matrix operations. A couple other thoughts in that vein: creating the <code>mask</code> object is going to take some amount of memory; it may be better to sequentially generate random numbers in a single loop over the input. Also, if <code>deterministic</code> is true and <code>rescale</code> is true, you have to pass through the input matrix twice: once for <code>output = input</code> and once for <code>output *= scale</code>. I think it will be faster to do this:</p>
<pre><code>if (deterministic)
{
if (!rescale)
output = input;
else
output = input * scale;
}
</code></pre>
<p>Of course, it's probably worth running a quick test on either of those ideas to see if they're worth implementing or actually give a runtime improvement.</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/pull/463#issuecomment-151167047">view it on GitHub</a>.<img alt="" height="1" src="https://github.com/notifications/beacon/AJ4bFES4EGNCoyvJoMC40h_x2WYPkYPlks5o_jilgaJpZM4GSg7s.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/pull/463#issuecomment-151167047"></link>
<meta itemprop="name" content="View Pull Request"></meta>
</div>
<meta itemprop="description" content="View this Pull Request on GitHub"></meta>
</div>