Set lhs of underlying distance to cluster centers (#4388) #4510
Conversation
modified: src/shogun/clustering/Hierarchical.cpp
modified: src/shogun/clustering/Hierarchical.h
(Squashed)Made minor changes based on the suggestions of reviewers
| void CHierarchical::store_model_features() | ||
| { | ||
| /* TODO. Currently does nothing since apply methods are not implemented. */ | ||
| CDenseFeatures<float64_t>* rhs = | ||
| (distance->get_rhs())->as<CDenseFeatures<float64_t>>(); |
There was a problem hiding this comment.
I don't think you need the parenthesis here.
Also, are we really sure that the features are float64 bit?
There was a problem hiding this comment.
Will remove the parenthesis.
I checked the KmeansBase file and used float64 bit. I felt that it made sense to make it float64 bit. What other type do you suggest?
There was a problem hiding this comment.
KMeans is another algorithm. I just checked Hierarchical.cpp and it does not fix the features to be 64 bit float, but rather just calles the distance method of CDistance. So we will need to think a bit more here in order to not make the algorithm only work for those 64bit dense feautres. What about 32 bit? What about sparse features?` What about string features? But let's address the other points first, this one will take some time and effort
There was a problem hiding this comment.
Okay. I get your point. Thanks!
| CDenseFeatures<float64_t>* temp_cluster_centers = | ||
| new CDenseFeatures<float64_t>(null_matrix); | ||
|
|
||
| for (int32_t i = 0; i < num_vectors; i++) |
| int32_t centerIdx = assignment[i]; | ||
| SG_DEBUG("\n"); | ||
| SG_DEBUG("On %04i point, with assignment=%04i \n", i, centerIdx); | ||
| float64_t* center = temp_cluster_centers->get_feature_vector( |
There was a problem hiding this comment.
I think there should be a SGVector based API for feature vectors that you could use here. This pointer based one is deprecated
| center, num_features, dofree_c); | ||
| } | ||
|
|
||
| CDenseFeatures<float64_t>* cluster_centers = |
There was a problem hiding this comment.
I don't think the code to compute something should be in here. This is just a method to store a "smaller" version of the features, namely only those needed to apply the model to new data. If you need to compute things in order to do that, it should happen in a helper method
| curr_index, num_features, dofree); | ||
| SG_DEBUG("\n"); | ||
| SG_DEBUG("The %04i cluster center:\n", curr_index); | ||
| for (int32_t j = 0; j < num_features; j++) |
There was a problem hiding this comment.
pls use std::copy or similar for those operations, it is faster
| cluster_centers->free_feature_vector(center, num_features, dofree); | ||
| } | ||
| } | ||
| distance->init(cluster_centers, rhs); |
There was a problem hiding this comment.
you should add a unit test that
- Trains the clustering
- Applies to test data
- Calls store_model_features, asserts that now the features are different
- Now applies to test data again and assert that results didnt change
| { | ||
| if (n_cluster_samples[centerIdx] > 0) | ||
| { | ||
| center[j] = (center[j] * n_cluster_samples[centerIdx] + |
There was a problem hiding this comment.
you should leave a comment on what type of algorithm/approach is used here to store/compute model features
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
This issue is now being closed due to a lack of activity. Feel free to reopen it. |
No description provided.