<p><a href="https://github.com/keonkim" class="user-mention">@keonkim</a>  Looks good for me, only something need to refine</p>

<p>1 : format <br>
Split functions have some trailing spaces, please remove them</p>

<p>2 : move the train and test data into std::make_tuple<br>
The overload of Split function which return tuple, please move the trainData and testData into the tuple.</p>

<p><code>return std::make_tuple(std::move(trainData), std::move(testData));</code></p>

<p>3 : <code>if ((testRatio &lt; 0.0) &amp;&amp; (testRatio &gt; 1.0))</code></p>

<p>should be</p>

<p><code>if ((testRatio &lt; 0.0) || (testRatio &gt; 1.0))</code></p>

<p>4 :  Following codes will copy the value, you can</p>

<pre><code>mat training = get&lt;0&gt;(value);
mat test = get&lt;1&gt;(value);
mat trainingLabels = get&lt;2&gt;(value);
mat testLabels = get&lt;3&gt;(value);
</code></pre>

<p>a : move it</p>

<pre><code>mat training = std::move(get&lt;0&gt;(value));
mat test = std::move(get&lt;1&gt;(value));
mat trainingLabels = std::move(get&lt;2&gt;(value));
mat testLabels = std::move(get&lt;3&gt;(value));

</code></pre>

<p>b : use tuple directly</p>

<pre><code>data::Save(trainingFile, get&lt;0&gt;(value), false);
data::Save(testFile, get&lt;1&gt;(value), false);
data::Save(trainingLabelsFile, get&lt;2&gt;(value), false);
data::Save(testLabelsFile, get&lt;3&gt;(value), false);
</code></pre>

<p>c : use another api from the beginning</p>

<pre><code>mat training;
mat test;
mat trainingLabels;
mat testLabels;
data::Split(data, labels_row, training, test, trainingLabels, testLabels, testRatio);
</code></pre>

<p>d : take the reference, with const( I love const :) )</p>

<pre><code>mat const &amp;training = std::move(get&lt;0&gt;(value));
mat const &amp;test = std::move(get&lt;1&gt;(value));
mat const &amp;trainingLabels = std::move(get&lt;2&gt;(value));
mat const &amp;testLabels = std::move(get&lt;3&gt;(value));

</code></pre>

<p>split without parameters part has the same problem</p>

<p>5 : The test of non-labels Split haven't finished</p>

<p>The file only test the size after split, but do not test the data after are split into correct clusters(train and test). To test it, I think we could </p>

<p>step 1 : generate sequential, unique values to the input data<br>
step 2 : split input data into train data and test data<br>
step 3 : make sure all of the data in input data could be found in train data and test data</p>

<p style="font-size:small;-webkit-text-size-adjust:none;color:#666;">&mdash;<br />You are receiving this because you are subscribed to this thread.<br />Reply to this email directly, <a href="https://github.com/mlpack/mlpack/pull/650#issuecomment-222366440">view it on GitHub</a>, or <a href="https://github.com/notifications/unsubscribe/AJ4bFJ-IsNseEEJ5U_dLdNJ-Sz3uEBtKks5qGbCXgaJpZM4IneDD">mute the thread</a>.<img alt="" height="1" src="https://github.com/notifications/beacon/AJ4bFFX0Qm8fnwX5fEG14rfBVf_KeWs8ks5qGbCXgaJpZM4IneDD.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/650#issuecomment-222366440"></link>
  <meta itemprop="name" content="View Pull Request"></meta>
</div>
<meta itemprop="description" content="View this Pull Request on GitHub"></meta>
</div>