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

Fix issue #3612 #4127

Merged
merged 16 commits into from
Jan 29, 2018
Merged

Fix issue #3612 #4127

Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
549e646
First attempt at fixing #3612
Jan 25, 2018
574922b
Attempt #1 to fix segfault
Jan 27, 2018
9e8e5e2
Revert "Attempt #1 to fix segfault"
Jan 27, 2018
214c8e4
segafault fix attempt no2
Jan 27, 2018
c801712
Whitespace fix
Jan 27, 2018
be69f9a
Formatting fix
Jan 27, 2018
70ab831
Added that single space character to make the style checker happy
Jan 27, 2018
599d4ed
Added unref for the two subset stacks
Jan 27, 2018
02ed8a9
adds unit test functions for combined kernel subsetting behaviour. Fi…
Jan 28, 2018
72dfbb9
Formatting
Jan 28, 2018
deb551f
More formatting changes
Jan 28, 2018
ebea19f
indent fix
Jan 28, 2018
cb0aa15
removed a line to make the style checker happy
Jan 28, 2018
3205bde
Replaced explicit casts with dynamic casts
Jan 28, 2018
d831611
tab fixes
Jan 28, 2018
a8a50ad
:(
Jan 28, 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
195 changes: 141 additions & 54 deletions src/shogun/kernel/CombinedKernel.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,54 +61,34 @@ void CCombinedKernel::init_subkernel_weights()
eigen_wt = eigen_wt.array().exp();
set_subkernel_weights(wt);
}

bool CCombinedKernel::init(CFeatures* l, CFeatures* r)
bool CCombinedKernel::init_with_extracted_subsets(
CFeatures* l, CFeatures* r, SGVector<index_t> lhs_subset,
SGVector<index_t> rhs_subset)
{
if(enable_subkernel_weight_opt && !weight_update)
{
init_subkernel_weights();
}

/* if the specified features are not combined features, but a single other
* feature type, assume that the caller wants to use all kernels on these */
if (l && r && l->get_feature_class()==r->get_feature_class() &&
l->get_feature_type()==r->get_feature_type() &&
l->get_feature_class()!= C_COMBINED)
{
SG_DEBUG("Initialising combined kernel's combined features with the "
"same instance from parameters\n");
/* construct combined features with each element being the parameter */
CCombinedFeatures* combined_l=new CCombinedFeatures();
CCombinedFeatures* combined_r=new CCombinedFeatures();
for (index_t i=0; i<get_num_subkernels(); ++i)
{
combined_l->append_feature_obj(l);
combined_r->append_feature_obj(r);
}

/* recursive call with constructed combined kernel */
return init(combined_l, combined_r);
}

CKernel::init(l,r);
REQUIRE(l->get_feature_class()==C_COMBINED, "%s::init(): LHS features are"
" of class %s but need to be combined features!\n",
get_name(), l->get_name());
REQUIRE(r->get_feature_class()==C_COMBINED, "%s::init(): RHS features are"
" of class %s but need to be combined features!\n",
get_name(), r->get_name());
ASSERT(l->get_feature_type()==F_UNKNOWN)
ASSERT(r->get_feature_type()==F_UNKNOWN)

CFeatures* lf=NULL;
CFeatures* rf=NULL;
CKernel* k=NULL;

bool result=true;
CKernel::init(l, r);
REQUIRE(
Copy link
Member

Choose a reason for hiding this comment

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

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

l->get_feature_class() == C_COMBINED,
Copy link
Member

Choose a reason for hiding this comment

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

this segfaults if the caller passes nullptr

"%s::init(): LHS features are"
Copy link
Member

Choose a reason for hiding this comment

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

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

" of class %s but need to be combined features!\n",
get_name(), l->get_name());
REQUIRE(
r->get_feature_class() == C_COMBINED,
"%s::init(): RHS features are"
" of class %s but need to be combined features!\n",
get_name(), r->get_name());
ASSERT(l->get_feature_type() == F_UNKNOWN)
ASSERT(r->get_feature_type() == F_UNKNOWN)

CFeatures* lf = NULL;
CFeatures* rf = NULL;
CKernel* k = NULL;

bool result = true;
index_t f_idx = 0;

SG_DEBUG("Starting for loop for kernels\n")
for (index_t k_idx=0; k_idx<get_num_kernels() && result; k_idx++)
for (index_t k_idx = 0; k_idx < get_num_kernels() && result; k_idx++)
{
k = get_kernel(k_idx);

Expand All @@ -131,11 +111,13 @@ bool CCombinedKernel::init(CFeatures* l, CFeatures* r)
SG_UNREF(lf);
SG_UNREF(rf);
SG_UNREF(k);
SG_ERROR("CombinedKernel: Number of features/kernels does not match - bailing out\n")
SG_ERROR(
"CombinedKernel: Number of features/kernels does not "
"match - bailing out\n")
}

SG_DEBUG("Initializing 0x%p - \"%s\"\n", this, k->get_name())
result=k->init(lf,rf);
result = k->init(lf, rf);
SG_UNREF(lf);
SG_UNREF(rf);

Expand All @@ -144,13 +126,33 @@ bool CCombinedKernel::init(CFeatures* l, CFeatures* r)
}
else
{
SG_DEBUG("Initializing 0x%p - \"%s\" (skipping init, this is a CUSTOM kernel)\n", this, k->get_name())
SG_DEBUG(
"Initializing 0x%p - \"%s\" (skipping init, this is a CUSTOM "
"kernel)\n",
this, k->get_name())
if (!k->has_features())
SG_ERROR("No kernel matrix was assigned to this Custom kernel\n")
SG_ERROR(
"No kernel matrix was assigned to this Custom kernel\n")

// clear all previous subsets
((CCustomKernel*)k)->remove_all_row_subsets();
Copy link
Member

Choose a reason for hiding this comment

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

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?

// apply new subset
((CCustomKernel*)k)->add_row_subset(lhs_subset);

((CCustomKernel*)k)->remove_all_col_subsets();
// apply new subset
((CCustomKernel*)k)->add_col_subset(rhs_subset);

if (k->get_num_vec_lhs() != num_lhs)
SG_ERROR("Number of lhs-feature vectors (%d) not match with number of rows (%d) of custom kernel\n", num_lhs, k->get_num_vec_lhs())
SG_ERROR(
"Number of lhs-feature vectors (%d) not match with number "
"of rows (%d) of custom kernel\n",
num_lhs, k->get_num_vec_lhs())
if (k->get_num_vec_rhs() != num_rhs)
SG_ERROR("Number of rhs-feature vectors (%d) not match with number of cols (%d) of custom kernel\n", num_rhs, k->get_num_vec_rhs())
SG_ERROR(
"Number of rhs-feature vectors (%d) not match with number "
"of cols (%d) of custom kernel\n",
num_rhs, k->get_num_vec_rhs())
}

SG_UNREF(k);
Expand All @@ -169,15 +171,100 @@ bool CCombinedKernel::init(CFeatures* l, CFeatures* r)
return false;
}

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 ||
Copy link
Member

Choose a reason for hiding this comment

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

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...?

((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")

init_normalizer();
initialized=true;
initialized = true;
return true;
}

bool CCombinedKernel::init(CFeatures* l, CFeatures* r)
{
if (enable_subkernel_weight_opt && !weight_update)
{
init_subkernel_weights();
}
/*
* The two subsets, we will be passing those to init_with_extracted_subsets
Copy link
Member

Choose a reason for hiding this comment

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

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

*/
SGVector<index_t> lhs_subset;
SGVector<index_t> rhs_subset;

/*
* We will be passing these features to init_with_extracted_subsets
Copy link
Member

Choose a reason for hiding this comment

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

same as above

*/
CCombinedFeatures* combined_l;
CCombinedFeatures* combined_r;

/*
* Extract the subsets so that we can pass them on
Copy link
Member

Choose a reason for hiding this comment

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

and same again

*/
auto l_subset_stack = l->get_subset_stack();
auto r_subset_stack = r->get_subset_stack();

if (l_subset_stack->has_subsets())
{
lhs_subset = l_subset_stack->get_last_subset()->get_subset_idx();
}
else
{
lhs_subset = SGVector<index_t>(l->get_num_vectors());
lhs_subset.range_fill();
}

if (r_subset_stack->has_subsets())
{
rhs_subset = r_subset_stack->get_last_subset()->get_subset_idx();
}
else
{
rhs_subset = SGVector<index_t>(r->get_num_vectors());
rhs_subset.range_fill();
}

SG_UNREF(l_subset_stack);
SG_UNREF(r_subset_stack);

/* if the specified features are not combined features, but a single other
Copy link
Member

Choose a reason for hiding this comment

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

this is a good comment

* feature type, assume that the caller wants to use all kernels on these */
if (l && r && l->get_feature_class() == r->get_feature_class() &&
l->get_feature_type() == r->get_feature_type() &&
l->get_feature_class() != C_COMBINED)
{
SG_DEBUG(
"Initialising combined kernel's combined features with the "
"same instance from parameters\n");
/* construct combined features with each element being the parameter
* The we must make sure that we make any custom kernels aware of any
* subsets present!
*/
combined_l = new CCombinedFeatures();
combined_r = new CCombinedFeatures();

for (index_t i = 0; i < get_num_subkernels(); ++i)
{
combined_l->append_feature_obj(l);
combined_r->append_feature_obj(r);
}
}
else
{
/*
* Otherwise, we just pass l & r straight on
Copy link
Member

Choose a reason for hiding this comment

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

this can go

*/
combined_l = (CCombinedFeatures*)l;
combined_r = (CCombinedFeatures*)r;
}

return init_with_extracted_subsets(
combined_l, combined_r, lhs_subset, rhs_subset);
}

void CCombinedKernel::remove_lhs()
{
delete_optimization();
Expand Down Expand Up @@ -971,4 +1058,4 @@ CList* CCombinedKernel::combine_kernels(CList* kernel_list)
}

return return_list;
}
}
15 changes: 15 additions & 0 deletions src/shogun/kernel/CombinedKernel.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -458,6 +458,21 @@ class CCombinedKernel : public CKernel

private:
void init();
/**
* The purpose of this function is to make customkernels aware of any
* subsets present, regardless whether the features passed are of type
* CCombinedFeatures or not
* @param lhs combined features
* @param rhs rombined features
* @param lhs_subset subset present on lhs - pass identity subset if
* none
* @param rhs_subset subset present on rhs - pass identity subset if
* none
* @return init succesful
*/
bool init_with_extracted_subsets(
CFeatures* lhs, CFeatures* rhs, SGVector<index_t> lhs_subset,
SGVector<index_t> rhs_subset);

protected:
/** list of kernels */
Expand Down