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
Added CKernelDependenceMaximization and CBAHSIC in feature selection framework #2363
Conversation
@karlnapf @sejdino please have a look. I will add a python modular (as integration test) and libshogun examples for this next and compare it with other implementations. I'll think a bit about how to implement |
@@ -21,6 +21,7 @@ | |||
%newobject get_transposed(); | |||
%newobject create_merged_copy(CFeatures* other); | |||
%newobject copy_subset(SGVector<index_t> indices); | |||
%newobject copy_dimension_subset(SGVector<index_t> indices); |
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.
sure this thing even needs to be exposed to modular ?
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.
@karlnapf ummm.. well, its in public API of CFeatures.. so.. But since we have kept num_features thing out of CFeatures since it doesn't make sense for all feature types, maybe this method should not be here at all. Maybe a helper method in CFeatureSelection should handle it, like CFeatureSelection::get_num_features. What do you think?
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.
mmmh, no hiding in in such a specialised class is not a good idea. then rather keep it public and expose it. i just though nobody might ever call this, so rather hide to not confuse people. but maybe actually somebody wants to call it, so keep this stuff, sorry for the confusion :)
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.
@karlnapf well I thought that since our last discussion with @vigsterkr and @sonney2k regarding this num_feature thing, having a copy_dimension_subset in CFeatures would ultimately result in this method being unimplemented in all feature classes except for CDenseFeatures and CSparseFeatures :(
this is fine to merge! |
Added CKernelDependenceMaximization and CBAHSIC in feature selection framework
@karlnapf thanks for merging. I will add example in the next patch and also start a notebook on feature selection in shogun. |
cool! we are getting there! |
Addition/changes are:
apply_backward_elimination()
method frompublic
toprotected
. This is done to prevent unexpected results if someone calls this on, for example, an instance of FOHSIC. This shouldn't be allowed. This has to be done by public API viaCFeatureSelection::apply()
call.CDependenceMaximization::set_labels()
instead ofCDependenceMaximization::precompute()
. In case one just wants to observe the measures via publicCFeatureSelection::compute_measures()
call for experimental purpose, he should be able to do that without having to go throughprecompute()
which is for internal purpose.