Fix issue #3612 #4127

dhelekal · 2018-01-27T13:47:42Z

This PR addresses issue #3612, by allowing CombinedKernel init to properly subset CustomKernel subkernels. Hence CustomKernel should now work with mkl / x-validation

Test code used to verify the fix:

from shogun import *
from numpy import *

init_shogun_with_defaults()

feats_train=RealFeatures(random.rand(10,10))  
labels=BinaryLabels(array([1,1,1,1,1,-1,-1,-1,-1,-1]))   

kerg = GaussianKernel(2.0)

kerg.init(feats_train,feats_train)

ker1 = CustomKernel(kerg.get_kernel_matrix())
ker2 = GaussianKernel(2.0)

cker = CombinedKernel()

cker.append_kernel(ker1)
cker.append_kernel(ker2)

cft = CombinedFeatures()

cft.append_feature_obj(feats_train)
cft.append_feature_obj(feats_train)

cker.init(cft,cft)

libsvm = LibSVM()
svm = MKLClassification(libsvm)
svm.set_interleaved_optimization_enabled(False)
svm.set_kernel(cker)
svm.set_C_mkl(1000)
svm.set_mkl_norm(1)

splitting_strategy = StratifiedCrossValidationSplitting(labels, 3)
evaluation_criterium = AccuracyMeasure()
cross = CrossValidation(svm, cft, labels, splitting_strategy, evaluation_criterium)
cross.set_autolock(False)
cross.set_num_runs(2)

mkl_obs = ParameterObserverCV(True)
cross.subscribe_to_parameters(mkl_obs)

result = CrossValidationResult()
result = CrossValidationResult.obtain_from_generic(cross.evaluate())

obs = mkl_obs.get_observation(0)
fold = obs.get_fold(0)
machine = MKLClassification.obtain_from_generic(fold.get_trained_machine())

k = CombinedKernel.obtain_from_generic(machine.get_kernel())
k1 = k.get_kernel(0)
k2 = k.get_kernel(1)

print(k1.get_kernel_matrix())
print("\n")
print(k2.get_kernel_matrix())
print("\n")
print(k1.get_kernel_matrix()-k2.get_kernel_matrix())

Sample output for the test to verify subsetting works properly:
print(k1.get_kernel_matrix())

[[ 1.          0.50979882  0.49804497  0.28813449  0.27958548  0.30324122]
 [ 0.50979882  1.          0.38971996  0.27236992  0.29889661  0.40002599]
 [ 0.49804497  0.38971996  1.          0.25557584  0.49059519  0.41748011]
 [ 0.28813449  0.27236992  0.25557584  1.          0.48876277  0.16951311]
 [ 0.27958548  0.29889661  0.49059519  0.48876277  1.          0.53417933]
 [ 0.30324122  0.40002599  0.41748011  0.16951311  0.53417933  1.        ]]

print(k2.get_kernel_matrix())

[[ 1.          0.50979884  0.49804496  0.28813448  0.27958548  0.30324122]
 [ 0.50979884  1.          0.38971996  0.27236991  0.29889661  0.400026  ]
 [ 0.49804496  0.38971996  1.          0.25557583  0.49059518  0.41748012]
 [ 0.28813448  0.27236991  0.25557583  1.          0.48876277  0.1695131 ]
 [ 0.27958548  0.29889661  0.49059518  0.48876277  1.          0.53417934]
 [ 0.30324122  0.400026    0.41748012  0.1695131   0.53417934  1.        ]]

print(k1.get_kernel_matrix()-k2.get_kernel_matrix())

[[  0.00000000e+00  -1.68008413e-08   6.80469564e-09   9.58425667e-09
   -2.85334684e-09   6.21391272e-11]
 [ -1.68008413e-08   0.00000000e+00   5.66272729e-09   1.40209140e-08
    3.09241743e-09  -3.52935375e-09]
 [  6.80469564e-09   5.66272729e-09   0.00000000e+00   3.50433216e-09
    1.25876629e-08  -1.27479330e-08]
 [  9.58425667e-09   1.40209140e-08   3.50433216e-09   0.00000000e+00
   -6.48492471e-09   6.75771872e-09]
 [ -2.85334684e-09   3.09241743e-09   1.25876629e-08  -6.48492471e-09
    0.00000000e+00  -9.37992795e-09]
 [  6.21391272e-11  -3.52935375e-09  -1.27479330e-08   6.75771872e-09
   -9.37992795e-09   0.00000000e+00]]

This reverts commit 574922b.

karlnapf · 2018-01-27T15:09:30Z

src/shogun/kernel/CombinedKernel.cpp

+				auto rhs_subset =
+				    rhs_subset_stack->get_last_subset()->get_subset_idx();
+				// clear all previous subsets
+				((CCustomKernel*)k)->remove_all_col_subsets();


I think you shouldn’t clear the subsets here.
This should happen from outside (and only the last one) as otherwise you might remove stuff a user has set previously

I operated under the assumption that the subset should be maintained in sync with the combinedfeatures used to init the combinedkernel. Normally, as per my understanding, the subset for the subkernels is obtained from the features used to init them. (The init(features l, features r) method of custom kernel in fact clears the subsets).
As the custom kernel has no features in the 1st place, I attempted to replicate what an init() would do if it had features.
Alternative approach: I could instantiate CIndexFeatures using lhs_subset/rhs_subset, then init the custom kernel using these. This might actually be a better approach, on a second though

Update: There seems to be an inconsistency within CCustomKernel::init(CFeatures* l, CFeatures* r).
Namely, if l & r are of type CIndexFeatures the normalizer doesnt get called, see following block

if (l->get_feature_class()==C_INDEX && r->get_feature_class()==C_INDEX) { CIndexFeatures* l_idx = (CIndexFeatures*)l; CIndexFeatures* r_idx = (CIndexFeatures*)r; remove_all_col_subsets(); remove_all_row_subsets(); add_row_subset(l_idx->get_feature_index()); add_col_subset(r_idx->get_feature_index()); lhs_equals_rhs=m_is_symmetric; return true; } /* For other types of CFeatures do the default actions below */ CKernel::init(l, r); lhs_equals_rhs=m_is_symmetric; SG_DEBUG("num_vec_lhs: %d vs num_rows %d\n", l->get_num_vectors(), kmatrix.num_rows) SG_DEBUG("num_vec_rhs: %d vs num_cols %d\n", r->get_num_vectors(), kmatrix.num_cols) ASSERT(l->get_num_vectors()==kmatrix.num_rows) ASSERT(r->get_num_vectors()==kmatrix.num_cols) return init_normalizer();

Is this intentional or not?

Most things in there were written in demand if one specific use case, so probably no.

I think everything should be consistent between normal kernels and custom kernels.

So maybe we keep it like this.

Memory check is important before merging though

I'm not 100% certain what do you mean re: consistency; could you please clarify it to me, perhaps on irc?

Sorry. I meant do it the same way for custom kernels as for normal kernels (clearing the stack)

OK, so keep the current method, rather than providing dummy indexfeatures, and calling the customkernels init on those? (+ and fix the memory issues)

karlnapf

Great thanks for the fix

Definitely useful!
Did you check your changes wth valgrind? There are memory leaks since you don’t unref some of the object you obtain from getters.

I am a bit concerned about the clearing of subsets, as said, can’t you remove the last one from outside the function in xvalidation?

karlnapf · 2018-01-27T15:11:53Z

A minimal unit test that checks the matrices as in your output would be super helpful

…xes an issue in which combined kernels would not subset custom kernels if features other than combinedfeatures were passed The latter is achieved by replacing the previous recursive call with a call to a helper function made aware of any subsets contained.

dhelekal · 2018-01-28T19:28:16Z

Update; the fix turned out to be a bit larger than expected; mainly because the added unit test revealed subsetting combined kernel wouldnt behave as expected if custom kernels were contained, and the features passed during init were of other typed than combined features. In order to fix this it was necessary to make the method aware of any subsets present. Hence instead of the past recursive call, there's a call to a helper method which now contains the bulk of the old init, and in addition taking the subsets as arguments.

vigsterkr · 2018-01-28T20:15:47Z

src/shogun/kernel/CombinedKernel.cpp

+				    "No kernel matrix was assigned to this Custom kernel\n")
+
+			// clear all previous subsets
+			((CCustomKernel*)k)->remove_all_row_subsets();


instead of the explicit cast could we do a dynamic_cast here so that way we can clearly get a bad_cast exception in case of some weird k?

vigsterkr · 2018-01-28T20:16:39Z

src/shogun/kernel/CombinedKernel.cpp

-	if ( ((CCombinedFeatures*) l)->get_num_feature_obj()<=0 ||
-		((CCombinedFeatures*) l)->get_num_feature_obj() != ((CCombinedFeatures*) r)->get_num_feature_obj() )
-		SG_ERROR("CombinedKernel: Number of features/kernels does not match - bailing out\n")
+	if (((CCombinedFeatures*)l)->get_num_feature_obj() <= 0 ||


you can dynamic cast these into a var and then use them... one would of course suppose that the compiler would do this as well under the hood... but it might be easier to read the code...?

vigsterkr · 2018-01-28T20:17:36Z

thnx for the fix!

vigsterkr · 2018-01-28T20:33:33Z

src/shogun/kernel/CombinedKernel.cpp

@@ -84,6 +84,9 @@ bool CCombinedKernel::init_with_extracted_subsets(
 	CFeatures* rf = NULL;
 	CKernel* k = NULL;

+    auto l_combined = dynamic_cast<CCombinedFeatures*>(l);


imo this will be a formatting error :(

oh that's odd, no clue why my editor suddenly switched to spaces instead of tabs. (

vigsterkr · 2018-01-28T20:57:52Z

https://travis-ci.org/shogun-toolbox/shogun/jobs/334467279#L745
:(

karlnapf · 2018-02-02T14:43:16Z

src/shogun/kernel/CombinedKernel.cpp

-
-	bool result=true;
+	CKernel::init(l, r);
+	REQUIRE(


if you require this, you could have asked for those types in the signature of the method, no?

karlnapf · 2018-02-02T14:43:37Z

src/shogun/kernel/CombinedKernel.cpp

+	CKernel::init(l, r);
+	REQUIRE(
+	    l->get_feature_class() == C_COMBINED,
+	    "%s::init(): LHS features are"


we actually don't do "CLASSNAME::METHODNAME():" anymore

karlnapf · 2018-02-02T14:43:56Z

src/shogun/kernel/CombinedKernel.cpp

-	bool result=true;
+	CKernel::init(l, r);
+	REQUIRE(
+	    l->get_feature_class() == C_COMBINED,


this segfaults if the caller passes nullptr

karlnapf · 2018-02-02T14:44:18Z

src/shogun/kernel/CombinedKernel.cpp

+	CFeatures* rf = NULL;
+	CKernel* k = NULL;
+
+	auto l_combined = dynamic_cast<CCombinedFeatures*>(l);


why dynamic cast if you don't check whether it worked?

karlnapf · 2018-02-02T14:45:23Z

src/shogun/kernel/CombinedKernel.cpp

+		init_subkernel_weights();
+	}
+	/*
+	 * The two subsets, we will be passing those to init_with_extracted_subsets


usually, we try to not have comments like this, a bit superflous, the code tells the story :)

karlnapf · 2018-02-02T14:45:30Z

src/shogun/kernel/CombinedKernel.cpp

+	SGVector<index_t> rhs_subset;
+
+	/*
+	 * We will be passing these features to init_with_extracted_subsets


same as above

karlnapf · 2018-02-02T14:45:34Z

src/shogun/kernel/CombinedKernel.cpp

+	CCombinedFeatures* combined_r;
+
+	/*
+	 * Extract the subsets so that we can pass them on


and same again

karlnapf · 2018-02-02T14:45:56Z

src/shogun/kernel/CombinedKernel.cpp

+	SG_UNREF(l_subset_stack);
+	SG_UNREF(r_subset_stack);
+
+	/* if the specified features are not combined features, but a single other


this is a good comment

karlnapf · 2018-02-02T14:46:21Z

src/shogun/kernel/CombinedKernel.cpp

+	else
+	{
+		/*
+		 * Otherwise, we just pass l & r straight on


this can go

karlnapf · 2018-02-02T14:47:54Z

tests/unit/kernel/CombinedKernel_unittest.cc

+TEST(CombinedKernelTest, test_subset_mixed)
+{
+
+	CMeanShiftDataGenerator* gen = new CMeanShiftDataGenerator(0, 2);


if you use auto gen = some<CMeanShiftDataGenerator>(0,2);, then you dont need to SG_UNREF later on

karlnapf · 2018-02-02T14:48:44Z

tests/unit/kernel/CombinedKernel_unittest.cc

+		{
+			for (index_t k = 0; k < 10; ++k)
+			{
+				EXPECT_LE(


EXPECT_NEAR would be much cleaner

karlnapf · 2018-02-02T14:49:01Z

tests/unit/kernel/CombinedKernel_unittest.cc

+	SGVector<index_t> inds(10);
+	inds.range_fill();
+
+	for (index_t i = 0; i < 10; ++i)


why not put the 10 into a variable with a meaninful name?

karlnapf · 2018-02-02T14:50:43Z

tests/unit/kernel/CombinedKernel_unittest.cc

+	CMeanShiftDataGenerator* gen = new CMeanShiftDataGenerator(0, 2);
+	CFeatures* feats = gen->get_streamed_features(10);
+
+	CCombinedFeatures* cf = new CCombinedFeatures();


better name: feats_combined (good names avoid the need for code comments)

karlnapf · 2018-02-02T14:51:56Z

tests/unit/kernel/CombinedKernel_unittest.cc

+		combined->init(cf, cf);
+
+		CKernel* k_g = combined->get_kernel(1);
+		CKernel* k_0 = combined->get_kernel(0);


these names are confusing me

karlnapf · 2018-02-02T14:52:25Z

tests/unit/kernel/CombinedKernel_unittest.cc

+		SGMatrix<float64_t> custom_matrix_1 = k_0->get_kernel_matrix();
+		SGMatrix<float64_t> custom_matrix_2 = k_3->get_kernel_matrix();
+
+		for (index_t j = 0; j < 10; ++j)


I think SGMatrix has an equals method

karlnapf

Thanks for the patch!

I have made some comments that could go into a clean-up PR that would greatly increase the readability of this code

Fix issue shogun-toolbox#3612 adds unit test functions for combined kernel subsetting behaviour. Fixes an issue in which combined kernels would not subset custom kernels if features other than combinedfeatures were passed The latter is achieved by replacing the previous recursive call with a call to a helper function made aware of any subsets contained.

cleanup for #4127

Fix issue shogun-toolbox#3612 adds unit test functions for combined kernel subsetting behaviour. Fixes an issue in which combined kernels would not subset custom kernels if features other than combinedfeatures were passed The latter is achieved by replacing the previous recursive call with a call to a helper function made aware of any subsets contained.

cleanup for shogun-toolbox#4127

DavidH added 7 commits January 25, 2018 20:53

First attempt at fixing #3612

549e646

Attempt #1 to fix segfault

574922b

Revert "Attempt #1 to fix segfault"

9e8e5e2

This reverts commit 574922b.

segafault fix attempt no2

214c8e4

Whitespace fix

c801712

Formatting fix

be69f9a

Added that single space character to make the style checker happy

70ab831

karlnapf reviewed Jan 27, 2018

View reviewed changes

DavidH added 4 commits January 27, 2018 18:28

Added unref for the two subset stacks

599d4ed

Formatting

72dfbb9

More formatting changes

deb551f

DavidH added 2 commits January 28, 2018 19:45

indent fix

ebea19f

removed a line to make the style checker happy

cb0aa15

vigsterkr requested changes Jan 28, 2018

View reviewed changes

Replaced explicit casts with dynamic casts

3205bde

vigsterkr reviewed Jan 28, 2018

View reviewed changes

tab fixes

d831611

:(

a8a50ad

vigsterkr approved these changes Jan 29, 2018

View reviewed changes

vigsterkr merged commit eed22ff into shogun-toolbox:develop Jan 29, 2018

dhelekal deleted the feature/customkernel-subset-fix branch January 29, 2018 09:26

karlnapf reviewed Feb 2, 2018

View reviewed changes

src/shogun/kernel/CombinedKernel.cpp

else

{

/*

* Otherwise, we just pass l & r straight on

Copy link

Member

karlnapf Feb 2, 2018

Choose a reason for hiding this comment

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

this can go

karlnapf reviewed Feb 2, 2018

View reviewed changes

tests/unit/kernel/CombinedKernel_unittest.cc

{

for (index_t k = 0; k < 10; ++k)

{

EXPECT_LE(

Copy link

Member

karlnapf Feb 2, 2018

Choose a reason for hiding this comment

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

EXPECT_NEAR would be much cleaner

karlnapf reviewed Feb 2, 2018

View reviewed changes

vigsterkr pushed a commit that referenced this pull request Feb 6, 2018

Cleanup PR 4127 (#4153)

56ae5e1

cleanup for #4127

ktiefe pushed a commit to ktiefe/shogun that referenced this pull request Jul 30, 2019

Cleanup PR 4127 (shogun-toolbox#4153)

8262126

cleanup for shogun-toolbox#4127

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix issue #3612 #4127

Fix issue #3612 #4127

dhelekal commented Jan 27, 2018

karlnapf Jan 27, 2018

dhelekal Jan 27, 2018

dhelekal Jan 27, 2018

karlnapf Jan 27, 2018

dhelekal Jan 27, 2018

karlnapf Jan 27, 2018

dhelekal Jan 27, 2018

karlnapf left a comment

karlnapf commented Jan 27, 2018

dhelekal commented Jan 28, 2018

vigsterkr Jan 28, 2018

vigsterkr Jan 28, 2018

vigsterkr commented Jan 28, 2018

vigsterkr Jan 28, 2018

dhelekal Jan 28, 2018

vigsterkr commented Jan 28, 2018

karlnapf Feb 2, 2018

karlnapf Feb 2, 2018

karlnapf Feb 2, 2018

karlnapf Feb 2, 2018

karlnapf Feb 2, 2018

karlnapf Feb 2, 2018

karlnapf Feb 2, 2018

karlnapf Feb 2, 2018

karlnapf Feb 2, 2018

karlnapf Feb 2, 2018

karlnapf Feb 2, 2018

karlnapf Feb 2, 2018

karlnapf Feb 2, 2018

karlnapf Feb 2, 2018

karlnapf Feb 2, 2018

karlnapf left a comment

Fix issue #3612 #4127

Fix issue #3612 #4127

Conversation

dhelekal commented Jan 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karlnapf left a comment

Choose a reason for hiding this comment

karlnapf commented Jan 27, 2018

dhelekal commented Jan 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vigsterkr commented Jan 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vigsterkr commented Jan 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karlnapf left a comment

Choose a reason for hiding this comment