<p>It's probably worth taking a look at the semi-finished <a href="https://github.com/mlpack/mlpack/wiki/ProposedNewDesignGuidelines">https://github.com/mlpack/mlpack/wiki/ProposedNewDesignGuidelines</a> and the style guide at <a href="https://github.com/mlpack/mlpack/wiki/StyleGuidelines">https://github.com/mlpack/mlpack/wiki/StyleGuidelines</a>.</p>
<blockquote>
<p>1 : allow users to get the trained parameters of SparseAutoencoder and SoftmaxRegression<br>
ex :<br>
arma::mat parameters() const;<br>
void parameters(arma::mat const &parameters);</p>
</blockquote>
<p>Fine by me, but make sure to pass by reference:</p>
<pre><code>arma::mat& Parameters(); // for modification
const arma::mat& Parameters() const; // for access
</code></pre>
<blockquote>
<p>2 : provide a default constructor for class SparseAutoencoder and SoftmaxRegression<br>
This allow users to train the SparseAutoencoder or SoftmaxRegression later on, so the users do not need to wrap them in smart pointer just to delay the initialization time.</p>
</blockquote>
<p>We need to be slightly careful with this one. If we provide a default constructor, what should be returned is a valid sparse autoencoder or a valid softmax regression object. The reason for this is that we should avoid situations where a user can have an invalid <code>SparseAutoencoder</code> or <code>SoftmaxRegression</code> object. What do you think? I am still thinking about the right way to do default constructors (or how to handle default-constructed mlpack objects).</p>
<blockquote>
<p>3 : This one is related to suggestion 2, provide a train function for SparseAutoencoder and SoftmaxRegression, this way the users can train the data whenever they like</p>
</blockquote>
<p>Yes, this is a good idea. The constructors will be able to be refactored to call <code>Train()</code>, too. Some comments on your proposed functions below:</p>
<blockquote>
<p>ex :<br>
//for SoftMaxRegression</p>
<p>//this one will use a random initialPoint to train<br>
void train(arma::mat const &input, arma::vec const &label, size_t numOfLabels);<br>
//this one can setup the initialPoint<br>
void train(arma::mat const &input, arma::const &initialPoint,<br>
arma::vec const &label, size_t numOfLabels);</p>
</blockquote>
<p>At the point of calling <code>Train()</code>, the <code>SoftmaxRegression</code> model should already be a valid object. So setting the initial point for the training shouldn't be necessary; instead, <code>Train()</code> should use the current model parameters as the initial point for the training. As a nice side effect, this means that you can train on multiple datasets sequentially and get a coherent model in the end.</p>
<blockquote>
<p>template<br>
void train(OptimizerType& optimizer);<br>
template<br>
void train(OptimizerType& optimizer, arma::mat const &initialPoint);</p>
</blockquote>
<p>For this particular overload, it's worth pointing out that <code>optimizer</code> holds an <code>initialPoint</code> inside of it, so the initial point overload ends up being redundant here.</p>
<p>I'd also consider adding a single-point training function:</p>
<pre><code>template<typename VecType>
void Train(const VecType& point, const size_t label);
</code></pre>
<blockquote>
<p>//for SparseAutoencoder</p>
<p>//this one will use a random parameters to train<br>
void train(arma::mat const &data, size_t hiddenSize);<br>
//this one can reuse the trained parameter<br>
void train(const arma::mat &data, const arma::mat &parameters,<br>
size_t hiddenSize);</p>
</blockquote>
<p>See the comments on <code>SoftmaxRegression::Train()</code>---at this point, we should already have a coherent <code>SparseAutoencoder</code> model, so we should start our training from the current model parameters.</p>
<blockquote>
<p>4 : Add "m_" before the data member of the class, by now SoftMaxRegression and SparseAutoencoder will use the same name to initialize the data member.</p>
</blockquote>
<p>I disagree with this one; the style guidelines already suggest naming conventions, and then access to the internal parameters is simply done by capitalizing the first letter of the parameter. For instance, if a class holds a member <code>matrix</code>, then this can be accessed/modified with the <code>Matrix()</code> function. Also it would be a truly tremendous amount of work to refactor all mlpack code to adhere to a new naming scheme. :)</p>
<p>If you want to send pull requests for pieces at a time, I'll merge them in (as long as they're specific to the existing SparseAutoencoder and SoftmaxRegression code, otherwise it may have to wait on someone else's approval too).</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/454#issuecomment-143235049">view it on GitHub</a>.<img alt="" height="1" src="https://github.com/notifications/beacon/AJ4bFP6AxQ6FQziqcEDtAVwbGG5gxpf4ks5o1U4wgaJpZM4GDtwe.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/454#issuecomment-143235049"></link>
<meta itemprop="name" content="View Issue"></meta>
</div>
<meta itemprop="description" content="View this Issue on GitHub"></meta>
</div>