Add tests for command-line and Python bindings (nca) #1369

yaswagner · 2018-04-16T02:06:30Z

Tests for the NCA binding, to insure its inputs and outputs work correctly.
Added a nca_test.cpp file under maint_test.

daiviknema

Hi there, thanks for the contribution! It looks like a number of checks are failing. This is because the minimum version of Armadillo supported by mlpack is 6.500. Unfortunately, arma::approx_equal is not available till 6.700. You could replace all calls to approx_equal with CheckMatrices which is available in mlpack/tests/test_tools.hpp, or write a loop by hand. That should help pass a few checks.

rcurtin

Hi there Yasmine,

Thanks for the contribution. The tests you've written look good to me. It would be nice to also add another few simple tests that look a lot like the ones you've already written, if you are willing:

Make sure that if input and labels are given, that an exception is thrown in mlpackMain() if the matrices have different numbers of points.
When tolerance is changed, we should check that the results are different. We could also do this for linear_scan, batch_size, and some of the L-BFGS-specific parameters. But some of those L-BFGS parameters like max_line_search_trials won't actually be guaranteed (or even likely) to cause a difference in the output, so I don't think we should worry about all of them.

Let me know what you think. If you are able to add the other tests, that is great, and if not we can just handle the single issue with approx_equal() and make sure the tests pass and then merge.

Thanks again! 👍

rcurtin · 2018-04-20T19:03:13Z

src/mlpack/tests/main_tests/nca_test.cpp

+
+  // Check that the output matrices are different.
+  BOOST_REQUIRE_EQUAL(arma::approx_equal(
+      CLI::GetParam<arma::mat>("output"), output, "absdiff", 0), false);


To avoid the use of approx_equal() here you could also do BOOST_REQUIRE_GT(arma::accu(CLI::GetParam<arma::mat>("output") != output), 0);

…r batch_size, linear_scan, and tolerance.

rcurtin

Sorry, I didn't realize that this one was waiting on me!

Anyway, I think the tests look fine, so I will go ahead and merge this in 4 days to leave time for other comments. If you'd like to add your name to the list of contributors in src/mlpack/core.hpp and COPYRIGHT.txt, we can merge that too.

Also, if you want some mlpack stickers for your laptop, I'd be happy to mail them to you---just send me an email with your mailing address and I'll get them in the mail early next week (I am out of town over the weekend).

If you have a chance to look into the issues I mentioned (specifically the random seed issue), it would be great, but if not I'll handle it during merge. I think the tests are fine, that is just a minor issue to fix.

Thanks again for the contribution! 👍

rcurtin · 2018-04-23T15:13:32Z

src/mlpack/tests/main_tests/nca_test.cpp

+/**
+* Ensure that output is different when the tolerance is different.
+*/
+BOOST_AUTO_TEST_CASE(NCADiffferentToleranceTest)


There is a really minor spelling issue here with Diffferent but I can fix it when I do the merge, unless you get to it first. :)

rcurtin · 2018-05-11T20:56:22Z

src/mlpack/tests/main_tests/nca_test.cpp

+  // Set parameters and set normalize to true.
+  SetInputParam("input", std::move(x));
+  SetInputParam("labels", std::move(labels));
+  SetInputParam("normalize", true);


So, there is a little bit of trickiness here. NCA by default uses the SGD optimizer, and the SGD optimizer depends on the random seed since it chooses batches randomly. In this case, we want to ensure that normalization makes a difference, but as the code currently is, it's likely to give different results simply by virtue of the fact that the order in which batches are visited for SGD will be different.

I think a workaround we can use is for this type of test, we can set linear_scan to true, so that we will always visit points in the same order. (The linear_scan test is fine as-is, since there all we want to know is that we get a different result there. In fact, it might be useful to add another test where we run the same data twice with linear_scan set, and we should expect the same result.)

zoq · 2018-05-15T13:59:43Z

Perhaps, it's a good idea to edit the message above, and to send a private message.

zoq

Looks good to me, just pointed out some minor style issue.

zoq · 2018-05-15T14:02:59Z

src/mlpack/tests/main_tests/nca_test.cpp

+                             " 1.0  0.0 -1.0  1.0  0.0 -1.0 ";
+  arma::Row<size_t> labels = " 0    0    0    1    1    1   ";
+
+  // Set parameters with a small bath_size.


Minor issue; small batch_size.

zoq · 2018-05-15T14:04:16Z

src/mlpack/tests/main_tests/nca_test.cpp

+BOOST_FIXTURE_TEST_SUITE(NCAMainTest, NCATestFixture);
+
+/**
+* Ensure that, when labels are implicitily given with input,


Pedantic style issue; missing space at the beginning, this should align with the first * from the first line.

zoq · 2018-05-15T14:05:01Z

src/mlpack/tests/main_tests/nca_test.cpp

+                              " 1.0  0.0 -1.0  1.0  0.0 -1.0 ";
+  arma::Row<size_t> labels2 = " 0    0    0    1    1    1   ";
+
+  // Set parameters using the same input but set linear_scan flag to false


Pedantic style issue number 2, missing . at the end.

zoq · 2018-05-15T14:05:36Z

src/mlpack/tests/main_tests/nca_test.cpp

+
+/**
+* Ensure that using a different value of max_iteration
+* results in a different output matrix


See comment above.

rcurtin · 2018-05-23T22:31:15Z

Thanks again for the contribution! I fixed a handful of things during the merge:

f5bee6e, 7d12b2b: added an explicit test for the label size to nca_main.cpp---the previous test would only succeed when compiled in debug mode.
93fa628: fixed a memory error I found with valgrind while running the tests (though that wasn't related to any code you wrote, I just found it and fixed it)
6ad2eaf: minor issue, the labels should have had 100 labels not 3 :)
e18d61d: I applied linear_scan to the rest of the tests that used SGD

Let me know if I broke anything or changed it in a way you don't like; I think the changes are mostly pretty minor though. 👍

Yasmine Dumouchel Wagner added 2 commits April 15, 2018 22:00

#1152 Add tests for command-line and Python bindings

f4abcb0

#1152 Add tests for command-line and Python bindings- fixed style issues

4b02518

daiviknema reviewed Apr 17, 2018

View reviewed changes

rcurtin reviewed Apr 20, 2018

View reviewed changes

Yasmine Dumouchel Wagner added 4 commits April 20, 2018 23:47

#1152 corrected previous test that used approx_equal. Created test fo…

d8ed987

…r batch_size, linear_scan, and tolerance.

#1152 took out the NCALabelSizeTest

58e1772

#1152 put back the NCALabelSizeTest

d623d15

#1152 took out the NCALabelSizeTest

06981e5

rcurtin approved these changes May 11, 2018

View reviewed changes

zoq approved these changes May 15, 2018

View reviewed changes

rcurtin mentioned this pull request May 16, 2018

Add tests for command-line and Python bindings #1152

Closed

39 tasks

yaswagner changed the title ~~#1152 Add tests for command-line and Python bindings~~ Add tests for command-line and Python bindings (nca) May 23, 2018

yaswagner added 2 commits May 22, 2018 21:34

Added test for NCA binding

757fe1c

NCA binding tests

d857114

rcurtin merged commit d857114 into mlpack:master May 23, 2018

yaswagner deleted the 1152 branch May 25, 2018 23:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add tests for command-line and Python bindings (nca) #1369

Add tests for command-line and Python bindings (nca) #1369

yaswagner commented Apr 16, 2018

daiviknema left a comment •

edited

rcurtin left a comment

rcurtin Apr 20, 2018

rcurtin left a comment

rcurtin Apr 23, 2018

rcurtin May 11, 2018

zoq commented May 15, 2018

zoq left a comment

zoq May 15, 2018

zoq May 15, 2018

zoq May 15, 2018

zoq May 15, 2018

rcurtin commented May 23, 2018

Add tests for command-line and Python bindings (nca) #1369

Add tests for command-line and Python bindings (nca) #1369

Conversation

yaswagner commented Apr 16, 2018

daiviknema left a comment • edited

Choose a reason for hiding this comment

rcurtin left a comment

Choose a reason for hiding this comment

rcurtin Apr 20, 2018

Choose a reason for hiding this comment

rcurtin left a comment

Choose a reason for hiding this comment

rcurtin Apr 23, 2018

Choose a reason for hiding this comment

rcurtin May 11, 2018

Choose a reason for hiding this comment

zoq commented May 15, 2018

zoq left a comment

Choose a reason for hiding this comment

zoq May 15, 2018

Choose a reason for hiding this comment

zoq May 15, 2018

Choose a reason for hiding this comment

zoq May 15, 2018

Choose a reason for hiding this comment

zoq May 15, 2018

Choose a reason for hiding this comment

rcurtin commented May 23, 2018

daiviknema left a comment •

edited