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.

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

Support cross validation of pipeline #4377

Merged
merged 8 commits into from Aug 8, 2018

Conversation

vinx13
Copy link
Member

@vinx13 vinx13 commented Jul 17, 2018

  • implemented store_model_features in pipeline, which forwards the call to the underlying machine of the pipeline
    * made store_model_feature public, such that as a method of base class CMachine it is callable from pipeline
  • overrode clone to make pipeline cloneable
  • overrode get_machine_problem_type

@vinx13 vinx13 changed the title made store_model_features public and implemented it in pipeline Support cross validation of pipeline Jul 17, 2018
@vinx13 vinx13 force-pushed the feature/pipeline_xval branch 2 times, most recently from 457aac1 to f508903 Compare July 27, 2018 05:59
@@ -108,6 +108,11 @@ class CHierarchical : public CDistanceMachine
return false;
}

/** TODO: Ensures cluster centers are in lhs of underlying distance
Copy link
Member

Choose a reason for hiding this comment

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

could you open an issue with this?

Copy link
Member Author

@vinx13 vinx13 Jul 31, 2018

Choose a reason for hiding this comment

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

this comment was left before, i just copy-pasted to make the method public

Copy link
Member

Choose a reason for hiding this comment

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

yeah I know, just so that it is documented. This is a nice entry task

@karlnapf
Copy link
Member

couldnt we make some friend classes instead of adding the set_store_model features to the interface. This is not meant to be an interface method (definitely not for SWIG, but not even for cpp)

* and therefore the model anyway
*/
virtual void store_model_features();
/** Computes the added bias. The bias is computed
Copy link
Member

Choose a reason for hiding this comment

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

wasnt this method remove in develop?


void CPipeline::set_store_model_features(bool store_model)
{
get_machine()->set_store_model_features(store_model);
Copy link
Member

Choose a reason for hiding this comment

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

If Pipeline inherits from CMachine, why cant the method be protected?

Copy link
Member Author

Choose a reason for hiding this comment

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

it's a protected method from base class, you cannot access it of another instance in the derived class CPipeline

Copy link
Member

Choose a reason for hiding this comment

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

sorry of course you are right. friend class?

CSGObject* CPipeline::clone()
{
auto result = CMachine::clone()->as<CPipeline>();
for (auto&& stage : m_stages)
Copy link
Member

Choose a reason for hiding this comment

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

I think we could probably do this via the parameter framework clone, with a few additions for std::types. The class is not serializable anyways.
@lisitsyn what do you think?

Copy link
Member

@karlnapf karlnapf left a comment

Choose a reason for hiding this comment

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

LGTM, apart from this public method that should be at least hidden from swig, or better, not be public.

@vinx13 vinx13 force-pushed the feature/pipeline_xval branch 2 times, most recently from 767990c to 1079fb9 Compare July 31, 2018 18:04
Copy link
Member

@karlnapf karlnapf left a comment

Choose a reason for hiding this comment

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

Good from my side

@vinx13
Copy link
Member Author

vinx13 commented Aug 6, 2018

@vigsterkr could you check and merge this?

@vigsterkr vigsterkr merged commit 2b0cbb9 into shogun-toolbox:develop Aug 8, 2018
ktiefe pushed a commit to ktiefe/shogun that referenced this pull request Jul 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants