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 and optimize transformers #4348

Merged
merged 6 commits into from Jul 11, 2018

Conversation

Projects
None yet
3 participants
@vinx13
Contributor

vinx13 commented Jun 28, 2018

  • Use linalg in NormOne
  • Optimize PruneVarSubMean

@vinx13 vinx13 changed the title from Cleanup and optimize code transformers to Cleanup and optimize transformers Jun 28, 2018

@vigsterkr vigsterkr force-pushed the shogun-toolbox:feature/transformers branch from 550639e to af8af72 Jun 28, 2018

@karlnapf

This comment has been minimized.

Member

karlnapf commented Jun 28, 2018

This needs a rebase

@karlnapf

This comment has been minimized.

Member

karlnapf commented Jun 28, 2018

Btw whenever you make something linalg, pls ensure there is some test coverage of the changes lines, so that we at least know he results didn’t change in the integration test examples

@vinx13 vinx13 force-pushed the vinx13:feature/transformers_update branch from dedcb4a to bdacb07 Jun 28, 2018

EXPECT_EQ(v.vlen, num_features);
for (auto j : range(v.vlen))
{
EXPECT_DOUBLE_EQ(v[j], data[num_features * i + j] / norm[i]);

This comment has been minimized.

@karlnapf

karlnapf Jun 29, 2018

Member

cant we use operator() here?

{
SGVector<float64_t> v = feats->get_feature_vector(i);
auto result = transformer->apply_to_feature_vector(v);
EXPECT_EQ(result.vlen, num_features);

This comment has been minimized.

@karlnapf

karlnapf Jun 29, 2018

Member

ASSERT_EQ to avoid segfault

@@ -102,7 +102,7 @@ void CPruneVarSubMean::cleanup()
SGMatrix<float64_t>
CPruneVarSubMean::apply_to_matrix(SGMatrix<float64_t> matrix)
{
ASSERT(m_initialized)
REQUIRE(m_initialized, "Transformer has not been fitted.\n");

This comment has been minimized.

@karlnapf

karlnapf Jun 29, 2018

Member

offtopic: I would love to move all those general checks to base classes so that they dont get repeated in every specialization....
any thoughts on this

This comment has been minimized.

@vigsterkr

vigsterkr Jul 11, 2018

Member

shouldn't we use a different exception type other than ShogunException

@vigsterkr vigsterkr force-pushed the shogun-toolbox:feature/transformers branch 2 times, most recently from 77a9f85 to 8711ea6 Jul 10, 2018

@vinx13 vinx13 force-pushed the vinx13:feature/transformers_update branch from c1b0299 to 47363cb Jul 10, 2018

@vinx13 vinx13 force-pushed the vinx13:feature/transformers_update branch from 47363cb to 918fead Jul 11, 2018

@@ -102,7 +102,7 @@ void CPruneVarSubMean::cleanup()
SGMatrix<float64_t>
CPruneVarSubMean::apply_to_matrix(SGMatrix<float64_t> matrix)
{
ASSERT(m_initialized)
REQUIRE(m_initialized, "Transformer has not been fitted.\n");

This comment has been minimized.

@vigsterkr

vigsterkr Jul 11, 2018

Member

shouldn't we use a different exception type other than ShogunException

@@ -132,31 +132,18 @@ CPruneVarSubMean::apply_to_matrix(SGMatrix<float64_t> matrix)
/// result in feature matrix
SGVector<float64_t> CPruneVarSubMean::apply_to_feature_vector(SGVector<float64_t> vector)
{
float64_t* ret=NULL;
REQUIRE(m_initialized, "Transformer has not been fitted.\n");

This comment has been minimized.

@vigsterkr

vigsterkr Jul 11, 2018

Member

shouldn't we use a different exception type other than ShogunException

MS_NOT_AVAILABLE);
}
void CTransformer::check_fitted() const

This comment has been minimized.

@vigsterkr

vigsterkr Jul 11, 2018

Member

i would rather go with the java like: is_fitted or is_fit depending which english we'd like to go with :) but let's stick to british (non north american english as in case of the exception type naming)

CTransformer::CTransformer() : CSGObject()
{
SG_ADD(
&m_fitted, "fitted", "Whether the transformer has been fitted.",

This comment has been minimized.

@vigsterkr

vigsterkr Jul 11, 2018

Member

let's register this param as is_fitted

@vigsterkr vigsterkr merged commit a241ab1 into shogun-toolbox:feature/transformers Jul 11, 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