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

Cleanup non-const methods in dense and combined features #4365

Merged
merged 3 commits into from Jul 10, 2018

Conversation

Projects
None yet
3 participants
@vinx13
Contributor

vinx13 commented Jul 9, 2018

  • methods made const: create_merged_copy, get_feature_matrix, get_feature_obj
  • dropped public methods: set_feature_vector, reshape, steal_feature_matrix
  • made free_features protected
@lisitsyn

This comment has been minimized.

Member

lisitsyn commented Jul 9, 2018

lgtm!

m_is_trained=true;
distance->init(m_centroids,distance->get_rhs());
distance->init(centroids_feats, distance->get_rhs());

This comment has been minimized.

@karlnapf

karlnapf Jul 9, 2018

Member

why the change here?

This comment has been minimized.

@vinx13

vinx13 Jul 9, 2018

Contributor

since we would like to drop setters in featuresset_num_vectors, set_num_features, we can't an empty features and then set feature matrix later, and here there is no need to store it in a member field

@@ -76,7 +76,7 @@ class CCombinedFeatures : public CFeatures
}
/** list feature objects */
void list_feature_objs();
void list_feature_objs() const;

This comment has been minimized.

@karlnapf

karlnapf Jul 9, 2018

Member

lol, this method should rather be deleted (or return a list of strings or so)

@@ -170,28 +170,6 @@ template<class ST> ST* CDenseFeatures<ST>::get_feature_vector(int32_t num, int32
return feat;
}
template<class ST> void CDenseFeatures<ST>::set_feature_vector(SGVector<ST> vector, int32_t num)

This comment has been minimized.

@karlnapf

karlnapf Jul 9, 2018

Member

it is not used?

This comment has been minimized.

@vinx13

vinx13 Jul 9, 2018

Contributor

yes, and i think set_feature_matrix should be also removed (after some refactor)

This comment has been minimized.

@karlnapf

karlnapf Jul 9, 2018

Member

absolutely, the instance should be tied to the matrix, and otherwise users can create a new instance

@@ -331,16 +309,6 @@ void CDenseFeatures<ST>::copy_feature_matrix(SGMatrix<ST> target, index_t column
}
}
template<class ST> SGMatrix<ST> CDenseFeatures<ST>::steal_feature_matrix()

This comment has been minimized.

@karlnapf

karlnapf Jul 9, 2018

Member

lol another joker method :D

@@ -468,21 +438,6 @@ template<class ST> void CDenseFeatures<ST>::initialize_cache()
template<class ST> EFeatureClass CDenseFeatures<ST>::get_feature_class() const { return C_DENSE; }
template<class ST> bool CDenseFeatures<ST>::reshape(int32_t p_num_features, int32_t p_num_vectors)

This comment has been minimized.

@karlnapf

karlnapf Jul 9, 2018

Member

this should just be in the matrix itself, i.e. create a new feature instance using the reshaped sgmatrix

@@ -259,7 +259,7 @@ class CFeatures : public CSGObject
* @return new feature object which contains copy of data of this
* instance and of given one
*/
virtual CFeatures* create_merged_copy(CFeatures* other)
virtual CFeatures* create_merged_copy(CFeatures* other) const

This comment has been minimized.

@karlnapf

karlnapf Jul 9, 2018

Member

maybe we can rename this to merge at some point later?

@karlnapf

good!

@karlnapf

This comment has been minimized.

Member

karlnapf commented Jul 9, 2018

CI is not happy though

@karlnapf

This comment has been minimized.

Member

karlnapf commented Jul 9, 2018

let me know once CI is happy and I will merge

@vinx13

This comment has been minimized.

Contributor

vinx13 commented Jul 10, 2018

@karlnapf timeout on windows

@karlnapf

This comment has been minimized.

Member

karlnapf commented Jul 10, 2018

restarted.
But I think this is good. Merging

@karlnapf karlnapf merged commit f10f3ef into shogun-toolbox:develop Jul 10, 2018

0 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment