-
-
Notifications
You must be signed in to change notification settings - Fork 1k
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.
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
Fix issue #3612 #4127
Fix issue #3612 #4127
Changes from all commits
549e646
574922b
9e8e5e2
214c8e4
c801712
be69f9a
70ab831
599d4ed
02ed8a9
72dfbb9
deb551f
ebea19f
cb0aa15
3205bde
d831611
a8a50ad
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -61,54 +61,37 @@ 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( | ||
l->get_feature_class() == C_COMBINED, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this segfaults if the caller passes |
||
"%s::init(): LHS features are" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
|
||
auto l_combined = dynamic_cast<CCombinedFeatures*>(l); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why dynamic cast if you don't check whether it worked? |
||
auto r_combined = dynamic_cast<CCombinedFeatures*>(r); | ||
|
||
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); | ||
|
||
|
@@ -118,11 +101,11 @@ bool CCombinedKernel::init(CFeatures* l, CFeatures* r) | |
// skip over features - the custom kernel does not need any | ||
if (k->get_kernel_type() != K_CUSTOM) | ||
{ | ||
if (((CCombinedFeatures*)l)->get_num_feature_obj() > f_idx && | ||
((CCombinedFeatures*)r)->get_num_feature_obj() > f_idx) | ||
if (l_combined->get_num_feature_obj() > f_idx && | ||
r_combined->get_num_feature_obj() > f_idx) | ||
{ | ||
lf = ((CCombinedFeatures*)l)->get_feature_obj(f_idx); | ||
rf = ((CCombinedFeatures*)r)->get_feature_obj(f_idx); | ||
lf = l_combined->get_feature_obj(f_idx); | ||
rf = r_combined->get_feature_obj(f_idx); | ||
} | ||
|
||
f_idx++; | ||
|
@@ -131,11 +114,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); | ||
|
||
|
@@ -144,13 +129,35 @@ 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") | ||
|
||
auto k_custom = dynamic_cast<CCustomKernel*>(k); | ||
|
||
// clear all previous subsets | ||
k_custom->remove_all_row_subsets(); | ||
// apply new subset | ||
k_custom->add_row_subset(lhs_subset); | ||
|
||
k_custom->remove_all_col_subsets(); | ||
// apply new subset | ||
k_custom->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); | ||
|
@@ -169,15 +176,99 @@ 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 (l_combined->get_num_feature_obj() <= 0 || | ||
l_combined->get_num_feature_obj() != r_combined->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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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(); | ||
|
@@ -971,4 +1062,4 @@ CList* CCombinedKernel::combine_kernels(CList* kernel_list) | |
} | ||
|
||
return return_list; | ||
} | ||
} |
There was a problem hiding this comment.
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?