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

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

Merged
merged 3 commits into from Jul 10, 2018
Merged

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
66fb2f7
Cleanup non-const methods in DenseFeatures
vinx13 Jul 9, 2018
7db59ef
Make several getters in CombinedFeatures const
vinx13 Jul 9, 2018
8134ad9
Drop set_feature_vector in python examples
vinx13 Jul 9, 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
14 changes: 2 additions & 12 deletions src/shogun/classifier/NearestCentroid.cpp
View file
Open in desktop
Expand Up @@ -29,17 +29,12 @@ namespace shogun{

CNearestCentroid::~CNearestCentroid()
{
if(m_is_trained)
distance->remove_lhs();
else
delete m_centroids;
}

void CNearestCentroid::init()
{
m_shrinking=0;
m_is_trained=false;
m_centroids = new CDenseFeatures<float64_t>();
}


Expand All @@ -65,9 +60,6 @@ namespace shogun{
SGMatrix<float64_t> centroids(num_feats,num_classes);
centroids.zero();

m_centroids->set_num_features(num_feats);
m_centroids->set_num_vectors(num_classes);

int64_t* num_per_class = new int64_t[num_classes];
for (int32_t i=0 ; i<num_classes ; i++)
{
Expand Down Expand Up @@ -100,12 +92,10 @@ namespace shogun{
SGVector<float64_t>::scale_vector(scale,target,num_feats);
}

m_centroids->free_feature_matrix();
m_centroids->set_feature_matrix(centroids);

auto centroids_feats = some<CDenseFeatures<float64_t>>(centroids);

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

Choose a reason for hiding this comment

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

why the change here?

Copy link
Member Author

Choose a reason for hiding this comment

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

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


SG_FREE(num_per_class);

Expand Down
14 changes: 7 additions & 7 deletions src/shogun/features/CombinedFeatures.cpp
View file
Open in desktop
Expand Up @@ -41,15 +41,15 @@ CCombinedFeatures::~CCombinedFeatures()
SG_UNREF(feature_array);
}

CFeatures* CCombinedFeatures::get_feature_obj(int32_t idx)
CFeatures* CCombinedFeatures::get_feature_obj(int32_t idx) const
{
REQUIRE(
idx < get_num_feature_obj() && idx>=0, "Feature index (%d) must be within [%d, %d]",
idx, 0, get_num_feature_obj()-1);
return (CFeatures*) feature_array->get_element(idx);
}

void CCombinedFeatures::list_feature_objs()
void CCombinedFeatures::list_feature_objs() const
{
SG_INFO("BEGIN COMBINED FEATURES LIST - ")
this->list_feature_obj();
Expand Down Expand Up @@ -111,12 +111,12 @@ bool CCombinedFeatures::check_feature_obj_compatibility(CCombinedFeatures* comb_
return result;
}

CFeatures* CCombinedFeatures::get_first_feature_obj()
CFeatures* CCombinedFeatures::get_first_feature_obj() const
{
return get_feature_obj(0);
}

CFeatures* CCombinedFeatures::get_last_feature_obj()
CFeatures* CCombinedFeatures::get_last_feature_obj() const
{
return get_feature_obj(get_num_feature_obj()-1);
}
Expand Down Expand Up @@ -159,7 +159,7 @@ bool CCombinedFeatures::delete_feature_obj(int32_t idx)
return feature_array->delete_element(idx);
}

int32_t CCombinedFeatures::get_num_feature_obj()
int32_t CCombinedFeatures::get_num_feature_obj() const
{
return feature_array->get_num_elements();
}
Expand All @@ -170,7 +170,7 @@ void CCombinedFeatures::init()
SG_ADD(&feature_array, "array", "Feature array.", MS_NOT_AVAILABLE);
}

CFeatures* CCombinedFeatures::create_merged_copy(CFeatures* other)
CFeatures* CCombinedFeatures::create_merged_copy(CFeatures* other) const
{
/* TODO, if all features are the same, only one copy should be created
* in memory */
Expand All @@ -185,7 +185,7 @@ CFeatures* CCombinedFeatures::create_merged_copy(CFeatures* other)
get_name());
}

CCombinedFeatures* casted=dynamic_cast<CCombinedFeatures*>(other);
auto casted = dynamic_cast<const CCombinedFeatures*>(other);

if (!casted)
{
Expand Down
12 changes: 6 additions & 6 deletions src/shogun/features/CombinedFeatures.h
View file
Open in desktop
Expand Up @@ -76,7 +76,7 @@ class CCombinedFeatures : public CFeatures
}

/** list feature objects */
void list_feature_objs();
void list_feature_objs() const;
Copy link
Member

Choose a reason for hiding this comment

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

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


/** check feature object compatibility
*
Expand All @@ -89,20 +89,20 @@ class CCombinedFeatures : public CFeatures
*
* @return first feature object
*/
CFeatures* get_first_feature_obj();
CFeatures* get_first_feature_obj() const;

/** get feature object at index idx
*
* @param idx index of feature object
* @return the feature object at index idx
*/
CFeatures* get_feature_obj(int32_t idx);
CFeatures* get_feature_obj(int32_t idx) const;

/** get last feature object
*
* @return last feature object
*/
CFeatures* get_last_feature_obj();
CFeatures* get_last_feature_obj() const;

/** insert feature object at index idx
* Important, idx must be < num_feature_obj
Expand Down Expand Up @@ -131,7 +131,7 @@ class CCombinedFeatures : public CFeatures
*
* @return number of feature objects
*/
int32_t get_num_feature_obj();
int32_t get_num_feature_obj() const;

/** Takes another feature instance and returns a new instance which is
* a concatenation of a copy if this instace's data and the given
Expand All @@ -143,7 +143,7 @@ class CCombinedFeatures : public CFeatures
* @return new feature object which contains copy of data of this
* instance and of given one
*/
CFeatures* create_merged_copy(CFeatures* other);
CFeatures* create_merged_copy(CFeatures* other) const;

/** adds a subset of indices on top of the current subsets (possibly
* subset o subset. Calls subset_changed_post() afterwards.
Expand Down
61 changes: 8 additions & 53 deletions src/shogun/features/DenseFeatures.cpp
View file
Open in desktop
Expand Up @@ -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)
Copy link
Member

Choose a reason for hiding this comment

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

it is not used?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

{
/* index conversion for subset, only for array access */
int32_t real_num=m_subset_stack->subset_idx_conversion(num);

if (num>=get_num_vectors())
{
SG_ERROR("Index out of bounds (number of vectors %d, you "
"requested %d)\n", get_num_vectors(), num);
}

if (!feature_matrix.matrix)
SG_ERROR("Requires a in-memory feature matrix\n")

if (vector.vlen != num_features)
SG_ERROR(
"Vector not of length %d (has %d)\n", num_features, vector.vlen);

sg_memcpy(&feature_matrix.matrix[real_num * int64_t(num_features)], vector.vector,
int64_t(num_features) * sizeof(ST));
}

template<class ST> SGVector<ST> CDenseFeatures<ST>::get_feature_vector(int32_t num)
{
/* index conversion for subset, only for array access */
Expand Down Expand Up @@ -286,7 +264,7 @@ template<class ST> void CDenseFeatures<ST>::feature_subset(int32_t* idx, int32_t
}

template <class ST>
SGMatrix<ST> CDenseFeatures<ST>::get_feature_matrix()
SGMatrix<ST> CDenseFeatures<ST>::get_feature_matrix() const
{
if (!m_subset_stack->has_subsets())
return feature_matrix;
Expand Down Expand Up @@ -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()
Copy link
Member

Choose a reason for hiding this comment

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

lol another joker method :D

{
SGMatrix<ST> st_feature_matrix=feature_matrix;
m_subset_stack->remove_all_subsets();
SG_UNREF(feature_cache);
clean_preprocessors();
free_feature_matrix();
return st_feature_matrix;
}

template<class ST> void CDenseFeatures<ST>::set_feature_matrix(SGMatrix<ST> matrix)
{
m_subset_stack->remove_all_subsets();
Expand All @@ -350,7 +318,9 @@ template<class ST> void CDenseFeatures<ST>::set_feature_matrix(SGMatrix<ST> matr
num_vectors = matrix.num_cols;
}

template<class ST> ST* CDenseFeatures<ST>::get_feature_matrix(int32_t &num_feat, int32_t &num_vec)
template <class ST>
ST* CDenseFeatures<ST>::get_feature_matrix(
int32_t& num_feat, int32_t& num_vec) const
{
num_feat = num_features;
num_vec = num_vectors;
Expand Down Expand Up @@ -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)
Copy link
Member

Choose a reason for hiding this comment

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

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

{
if (m_subset_stack->has_subsets())
SG_ERROR("A subset is set, cannot call reshape\n")

if (p_num_features * p_num_vectors
== this->num_features * this->num_vectors)
{
num_features = p_num_features;
num_vectors = p_num_vectors;
return true;
} else
return false;
}

template<class ST> int32_t CDenseFeatures<ST>::get_dim_feature_space() const { return num_features; }

template<class ST> float64_t CDenseFeatures<ST>::dot(int32_t vec_idx1, CDotFeatures* df,
Expand Down Expand Up @@ -1005,7 +960,7 @@ template<class ST> bool CDenseFeatures<ST>::is_equal(CDenseFeatures* rhs)
}

template <class ST>
CFeatures* CDenseFeatures<ST>::create_merged_copy(CList* others)
CFeatures* CDenseFeatures<ST>::create_merged_copy(CList* others) const
{
SG_DEBUG("Entering.\n");

Expand All @@ -1017,7 +972,7 @@ CFeatures* CDenseFeatures<ST>::create_merged_copy(CList* others)

while (current!=nullptr)
{
auto casted=dynamic_cast<CDenseFeatures<ST>*>(current);
auto casted = dynamic_cast<const CDenseFeatures<ST>*>(current);

REQUIRE(casted!=nullptr, "Provided object's type (%s) must match own type (%s)!\n",
current->get_name(), get_name());
Expand All @@ -1042,7 +997,7 @@ CFeatures* CDenseFeatures<ST>::create_merged_copy(CList* others)

while (current!=nullptr)
{
auto casted=static_cast<CDenseFeatures<ST>*>(current);
auto casted = static_cast<const CDenseFeatures<ST>*>(current);
casted->copy_feature_matrix(data, num_copied);
num_copied+=casted->get_num_vectors();

Expand All @@ -1059,7 +1014,7 @@ CFeatures* CDenseFeatures<ST>::create_merged_copy(CList* others)
}

template <class ST>
CFeatures* CDenseFeatures<ST>::create_merged_copy(CFeatures* other)
CFeatures* CDenseFeatures<ST>::create_merged_copy(CFeatures* other) const
{
auto list=some<CList>();
list->append_element(other);
Expand Down
53 changes: 13 additions & 40 deletions src/shogun/features/DenseFeatures.h
View file
Open in desktop
Expand Up @@ -113,12 +113,6 @@ template<class ST> class CDenseFeatures: public CDotFeatures
*/
void free_feature_matrix();

/** free feature matrix and cache
*
* Any subset is removed
*/
void free_features();

/** get feature vector
* for sample num from the matrix as it is if matrix is
* initialized, else return preprocessed compute_feature_vector (not
Expand All @@ -132,15 +126,6 @@ template<class ST> class CDenseFeatures: public CDotFeatures
*/
ST* get_feature_vector(int32_t num, int32_t& len, bool& dofree);

/** set feature vector num
*
* possible with subset
*
* @param vector vector
* @param num index if vector to set
*/
void set_feature_vector(SGVector<ST> vector, int32_t num);

/** get feature vector num
*
* possible with subset
Expand Down Expand Up @@ -206,15 +191,7 @@ template<class ST> class CDenseFeatures: public CDotFeatures
*
* @return matrix feature matrix
*/
SGMatrix<ST> get_feature_matrix();

/** steals feature matrix, i.e. returns matrix and
* forget about it
* subset is ignored
*
* @return matrix feature matrix
*/
SGMatrix<ST> steal_feature_matrix();
SGMatrix<ST> get_feature_matrix() const;

/** Setter for feature matrix
*
Expand All @@ -238,7 +215,7 @@ template<class ST> class CDenseFeatures: public CDotFeatures
* @param num_vec number of vectors in matrix
* @return feature matrix
*/
ST* get_feature_matrix(int32_t &num_feat, int32_t &num_vec);
ST* get_feature_matrix(int32_t& num_feat, int32_t& num_vec) const;

/** get a transposed copy of the features
*
Expand Down Expand Up @@ -317,16 +294,6 @@ template<class ST> class CDenseFeatures: public CDotFeatures
*/
virtual EFeatureType get_feature_type() const;

/** reshape
*
* not possible with subset
*
* @param p_num_features new number of features
* @param p_num_vectors new number of vectors
* @return if reshaping was successful
*/
virtual bool reshape(int32_t p_num_features, int32_t p_num_vectors);

/** obtain the dimensionality of the feature space
*
* (not mix this up with the dimensionality of the input space, usually
Expand Down Expand Up @@ -483,19 +450,19 @@ template<class ST> class CDenseFeatures: public CDotFeatures
* @return new feature object which contains copy of data of this
* instance and of given one
*/
CFeatures* create_merged_copy(CList* other);
CFeatures* create_merged_copy(CList* other) const;

/** Convenience method for method with same name and list as parameter.
*
* @param other feature object to append
* @return new feature object which contains copy of data of this
* instance and of given one
*/
CFeatures* create_merged_copy(CFeatures* other);
CFeatures* create_merged_copy(CFeatures* other) const;

/** helper method used to specialize a base class instance
*
*/
/** helper method used to specialize a base class instance
*
*/
#ifndef SWIG
[[deprecated("use .as template function")]]
#endif
Expand Down Expand Up @@ -523,6 +490,12 @@ template<class ST> class CDenseFeatures: public CDotFeatures
virtual ST* compute_feature_vector(int32_t num, int32_t& len,
ST* target = NULL);

/** free feature matrix and cache
*
* Any subset is removed
*/
void free_features();

private:
void init();

Expand Down