Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Refactored the CLibLinear test codes and reduced them from 1300 lines to 470. #4208

Merged
merged 9 commits into from
Apr 4, 2018
Merged

Refactored the CLibLinear test codes and reduced them from 1300 lines to 470. #4208

Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
76d1d24
Reduced the tests from 1300 lines to 470 using a few design patterns
FaroukY Mar 17, 2018
c7a4a20
Fixed the required changes discussed in pr #4208
FaroukY Mar 25, 2018
74d0b7d
Fixed the memory leaks by adding extra SG_REFS
FaroukY Apr 2, 2018
6163b81
Remove LAPACK requirement since it wasn't needed
FaroukY Apr 3, 2018
1affdc6
added BSD license, removed transpose function and replaced it with ge…
FaroukY Apr 3, 2018
4a65953
Fixed BSD authors
FaroukY Apr 3, 2018
473df52
Removed unnecessairy comments
FaroukY Apr 3, 2018
e54ce43
the function get_transposed already handles memory deallocation, so n…
FaroukY Apr 3, 2018
5a2580c
readded the SG_UNREF since get_transposed doesn't handle memory deall…
FaroukY Apr 3, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
68 changes: 23 additions & 45 deletions tests/unit/classifier/svm/LibLinear_unittest.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
/*
* This program is free software; you can redistribute it and/or modify
* it under the terms of the GNU General Public License as published by
* the Free Software Foundation; either version 3 of the License, or
* (at your option) any later version.
* This software is distributed under BSD 3-clause license (see LICENSE file).
*
* Written (W) 2014 pl8787
* Authors:
* Written (W) 2014 pl8787
* Refactored (R) 2018 Elfarouk Yasser
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just add yourself as author in the standard way...makes later processing of headers easier

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

*/

#include <shogun/classifier/svm/LibLinear.h>
#include <shogun/features/DataGenerator.h>
#include <shogun/features/DenseFeatures.h>
Expand Down Expand Up @@ -86,7 +86,7 @@ class LibLinear : public ::testing::Test
ll->set_bias_enabled(biasEnable);
ll->set_features(train_feats);
if (C_value)
ll->set_C(0.1,0.1); //Only in the case of L2R_L1LOSS_SVC_DUAL
ll->set_C(0.1,0.1); //Only in the case of L2R_L1LOSS_SVC_DUAL
ll->set_labels(ground_truth);
ll->set_liblinear_solver_type(liblinear_solver_type);
ll->train();
Expand All @@ -98,12 +98,12 @@ class LibLinear : public ::testing::Test

if (!biasEnable)
{
for(int i=0;i<t_w.vlen;i++)
for (auto i : range(t_w.vlen))
EXPECT_NEAR(ll->get_w()[i], t_w[i], 1e-5);
}
else
{
for(int i=0;i<t_w.vlen;i++)
for (auto i : range(t_w.vlen))
EXPECT_NEAR(ll->get_w()[i], t_w[i], 4e-5);
EXPECT_NEAR(ll->get_bias(), t_w[2], 4e-5);
}
Expand All @@ -115,19 +115,7 @@ class LibLinear : public ::testing::Test
}

protected:
void transpose_feats_matrix()
{
//Transpose train_feats matrix
SGMatrix<float64_t> train_matrix = train_feats->get_feature_matrix();
SGMatrix<float64_t>::transpose_matrix(train_matrix.matrix,
train_matrix.num_rows, train_matrix.num_cols);

SG_UNREF(train_feats);
train_feats = new CDenseFeatures<float64_t>(train_matrix);
SG_REF(train_feats);

}
void initialize_train_and_test_and_ground()
void generate_data_l2()
{
index_t num_samples = 50;
sg_rand->set_seed(5);
Expand Down Expand Up @@ -164,20 +152,17 @@ class LibLinear : public ::testing::Test
}
void generate_data_l1()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why this wrapper, you could just call the thing inside :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

{
initialize_train_and_test_and_ground();


transpose_feats_matrix();

}
void generate_data_l2()
{
initialize_train_and_test_and_ground();
generate_data_l2();
//Transpose the train_feats matrix
auto old_train_feats = train_feats;
train_feats = train_feats->get_transposed();
SG_UNREF(old_train_feats);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why the unref here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@karlnapf Doesn't get_transposed() create a new transposed matrix? So I'm just deallocating the old matrix (that i transposed)? Or does get_transposed() act in place?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess I would just delete the old one from the same scope that you created it in (tearDown or whatever), but this is also minor.

Make sure to run the test with valgrind to see whether the memory is fine btw

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ohh I see, get_transposed() handles the memory deallocation inside of it. So no need for SG_UNREF :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dont think it does ...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea i got a bit confused. Since I store the transposed matrix always in train_feats, I need to deallocate the memory before I leave the function or else we leak. The TearDown will release the transposed matrix (which is not stored in train_feats)

SG_REF(train_feats);
}
void initialize_train_and_test_and_ground_simple()
void generate_data_l2_simple()
{
SGMatrix<float64_t> train_data(2, 10);
SGMatrix<float64_t> test_data(2, 10); //Always 2x10, doesn't matter l1 or l2
SGMatrix<float64_t> test_data(2, 10);

index_t num_samples = 10;
SGVector<int32_t> x{0,0,1,1,2,2,3,3,4,4,5,5,6,6,7,7,8,8,9,9};
Expand Down Expand Up @@ -206,13 +191,12 @@ class LibLinear : public ::testing::Test
}
void generate_data_l1_simple()
{
initialize_train_and_test_and_ground_simple();

transpose_feats_matrix();
}
void generate_data_l2_simple()
{
initialize_train_and_test_and_ground_simple();
generate_data_l2_simple();
//transpose train_feats matrix
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the comment is useless ... super minor :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed it :)

auto old_train_feats = train_feats;
train_feats = train_feats->get_transposed();
SG_UNREF(old_train_feats);
SG_REF(train_feats);
}
};

Expand Down Expand Up @@ -316,12 +300,6 @@ TEST_F(LibLinear, train_L2R_LR_DUAL_BIAS)
train_with_solver(liblinear_solver_type, true, false);
}

/*
* --------------------------------
* Simple set tests start from here
* --------------------------------
*/

TEST_F(LibLinear, simple_set_train_L2R_LR)
{
LIBLINEAR_SOLVER_TYPE liblinear_solver_type = L2R_LR;
Expand Down