<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 < 0.0) && (testRatio > 1.0))</code></p>
<p>should be</p>
<p><code>if ((testRatio < 0.0) || (testRatio > 1.0))</code></p>
<p>4 : Following codes will copy the value, you can</p>
<pre><code>mat training = get<0>(value);
mat test = get<1>(value);
mat trainingLabels = get<2>(value);
mat testLabels = get<3>(value);
</code></pre>
<p>a : move it</p>
<pre><code>mat training = std::move(get<0>(value));
mat test = std::move(get<1>(value));
mat trainingLabels = std::move(get<2>(value));
mat testLabels = std::move(get<3>(value));
</code></pre>
<p>b : use tuple directly</p>
<pre><code>data::Save(trainingFile, get<0>(value), false);
data::Save(testFile, get<1>(value), false);
data::Save(trainingLabelsFile, get<2>(value), false);
data::Save(testLabelsFile, get<3>(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 &training = std::move(get<0>(value));
mat const &test = std::move(get<1>(value));
mat const &trainingLabels = std::move(get<2>(value));
mat const &testLabels = std::move(get<3>(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;">—<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>