Fix LMNN termination condition #4188
Conversation
|
Don't we have some unit tests to cover this? |
|
Otherwise fine from my side. |
|
@karlnapf This is not covered by unit tests, i think we can check the number of iterations. |
|
Yeah would be good to add something in these lines to this PR |
|
Added a tests. We need to fix trace first (#4176 ) to pass the test. |
|
Great! |
tests/unit/metric/LMNN_unittest.cc
Outdated
| @@ -62,6 +62,57 @@ TEST(LMNN,train_identity_init) | |||
| SG_UNREF(lmnn) | |||
| } | |||
|
|
|||
| TEST(LMNN, train_termination) | |||
| { | |||
| // create features, each column is a feature vector | |||
There was a problem hiding this comment.
could you remove all these superfluous comments?
There was a problem hiding this comment.
also, does this test share code with other tests? If so it should be pulled out and shared
There was a problem hiding this comment.
code that creates features and labels could be shared with other tests, but this could make it difficult to modify test input. shall we do this refactor?
tests/unit/metric/LMNN_unittest.cc
Outdated
|
|
||
| // check number of iterations | ||
| auto stat = lmnn->get_statistics(); | ||
| EXPECT_EQ(stat->obj.vlen, 1234); |
There was a problem hiding this comment.
Yes. I'm testing against Matlab prototype here https://github.com/iglesias/lmnn-gsoc2013-review
tests/unit/metric/LMNN_unittest.cc
Outdated
| // check number of iterations | ||
| auto stat = lmnn->get_statistics(); | ||
| EXPECT_EQ(stat->obj.vlen, 1234); | ||
| SG_UNREF(lmnn) |
There was a problem hiding this comment.
use some, then you don't need to unref
|
Once the tests pass this can be merged |
|
🎉 |
| @@ -292,6 +292,7 @@ bool CLMNNImpl::check_termination(float64_t stepsize, const SGVector<float64_t> | |||
|
|
|||
| if (iter >= 10) | |||
| { | |||
| obj_threshold *= obj[iter - 1]; | |||
There was a problem hiding this comment.
Nice catch @vinx13, thanks a lot for fixing.
* Fix LMNN termination condition * Add unit tests on lmnn termination
No description provided.