feature dispatching in base for CLARS #4350

shubham808 · 2018-06-29T15:32:19Z

[WIP] This is very minimal example of how we want dispatching to work based on the gist by @karlnapf . All feature classes and types are not covered.

karlnapf · 2018-06-30T12:00:31Z

src/shogun/machine/Machine.h

+  switch (data->get_feature_type()) \
+  { \
+    case F_DREAL: \
+      return train_templated_name<float64_t>(data->as<CDenseFeatures<float64_t>>()); \


I guess we could get away with static casting here as well, but I am always more comfortable using a dynamic cast as done in as

karlnapf · 2018-06-30T12:00:56Z

src/shogun/machine/Machine.h

@@ -315,7 +329,6 @@ class CMachine : public CStoppableSGObject

 		virtual const char* get_name() const { return "Machine"; }

-	protected:


you want train_machine to be public? why?

this is a mistake... no need for public ;)

karlnapf · 2018-06-30T12:02:21Z

src/shogun/machine/Machine.h

+					result=train_with_string(data);
+					break;
+				default:
+					SG_ERROR("train_machine is not yet implemented for %s!\n",


the error message should change and say that.
"Training %s with %s is not supported.\n", get_name(), data->get_name()

karlnapf · 2018-06-30T12:03:20Z

src/shogun/machine/Machine.h

-					get_name());
+			bool result=false;
+
+			switch (data->get_feature_class())


keep in mind data might be nullptr.
This actually raises a problem: as we cannot dispatch in that case ...
I guess we need to add a "default" option for training if features are not provided.
messy.
Or we just enforce the user to pass features from now on (API change that will have an impact all over shogun)
@vigsterkr @lisitsyn @iglesias thoughts?

@shubham808 I think for now you can add a "train_without_features" that could be overloaded by subclasses. We can then remove that later

default option will also work for features classes not yet in the switch cases, train_without_features makes more sense

karlnapf · 2018-06-30T12:07:02Z

src/shogun/regression/LeastAngleRegression.cpp

-	}
-	else
-		REQUIRE(data->get_feature_class() == C_DENSE,
-			"Feature-class must be of type C_DENSE (%d)\n", data->get_feature_class(), C_DENSE)


nice, so we shaved off all this code in every class that dispatches the templated type for training

karlnapf

Look good to me.
Why don't you add LDA as well so we get a feeling for how this works in there.

karlnapf · 2018-06-30T12:12:36Z

src/shogun/machine/Machine.h

@@ -25,6 +25,20 @@
 namespace shogun
 {

+#define TRAIN_DENSE_DISPATHER(train_templated_name) \
+virtual bool train_with_dense(CFeatures* data) \
+{  \


As of now, I am quite puzzled how we could combine this idea with iterative machine, since making the iteration method virtual and templated is not an option, but neither is to do the dispatching inside that method :(

Could we make the macro in a way that we can use it do overload the train_with_dense to dispatch the template type, but also that we can use it to overload iterate_dense ?

i think we will need a new macro for iterate_dense

the code is essentially the same, so why not try to create one macro for both.

accept the to be overloaded name as a parameter, here train_with_dense

accept a variable number of parameters that are passed on to the definition of the overloaded method and the call of the typed implementation

Then DENSE_DISPATHER(train_with_dense, train_templated) would expand to
virtual void train_with_dense(CFeatures* data) { ... train_templated(data); ... }

and DENSE_DISPATHER(iteration, iteration_templated) would expand to
virtual void iteration(CFeatures* data) { ... iteration_templated(data); ... }

i think iteration need not be implemented everywhere we have train_dense or ? (since dispatcher will be used in for eg LDA which is not an iterativemachine)

nope, but it would be cool to use the same macro

karlnapf · 2018-07-02T14:44:29Z

src/shogun/machine/Machine.h

@@ -25,6 +25,20 @@
 namespace shogun
 {

+#define TRAIN_DENSE_DISPATHER(train_templated_name) \


typo "DISPATCHER"

karlnapf · 2018-07-02T14:47:39Z

src/shogun/machine/Machine.h

+
+	protected:
+
+		virtual bool train_with_dense(CFeatures* data)


maybe an easier name would be train_dense

karlnapf · 2018-07-02T14:48:30Z

src/shogun/machine/Machine.h

+      return train_templated_name<float32_t>(data->as<CDenseFeatures<float32_t>>()); \
+		case F_LONGREAL: \
+	    return train_templated_name<floatmax_t>(data->as<CDenseFeatures<floatmax_t>>()); \
+  } \


needs a default case

karlnapf · 2018-08-06T14:19:57Z

Ill close this one as we solved this using CRTP instead of macros

feature dispatching in base for CLARS

ecb82e9

karlnapf reviewed Jun 30, 2018

View reviewed changes

karlnapf reviewed Jul 2, 2018

View reviewed changes

src/shogun/machine/Machine.h

protected:

virtual bool train_with_dense(CFeatures* data)

Copy link

Member

karlnapf Jul 2, 2018

Choose a reason for hiding this comment

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

maybe an easier name would be train_dense

karlnapf reviewed Jul 2, 2018

View reviewed changes

karlnapf closed this Aug 6, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature dispatching in base for CLARS #4350

feature dispatching in base for CLARS #4350

shubham808 commented Jun 29, 2018 •

edited

karlnapf Jun 30, 2018

karlnapf Jun 30, 2018

shubham808 Jun 30, 2018

karlnapf Jun 30, 2018

karlnapf Jun 30, 2018 •

edited

karlnapf Jun 30, 2018

shubham808 Jun 30, 2018

karlnapf Jun 30, 2018

karlnapf left a comment

karlnapf Jun 30, 2018

karlnapf Jul 2, 2018 •

edited

shubham808 Jul 2, 2018

karlnapf Jul 2, 2018 •

edited

shubham808 Jul 2, 2018 •

edited

karlnapf Jul 2, 2018

karlnapf Jul 2, 2018

karlnapf Jul 2, 2018

karlnapf Jul 2, 2018

karlnapf commented Aug 6, 2018

		@@ -315,7 +329,6 @@ class CMachine : public CStoppableSGObject

		virtual const char* get_name() const { return "Machine"; }

		protected:

feature dispatching in base for CLARS #4350

feature dispatching in base for CLARS #4350

Conversation

shubham808 commented Jun 29, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karlnapf Jun 30, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karlnapf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karlnapf Jul 2, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karlnapf Jul 2, 2018 • edited

Choose a reason for hiding this comment

shubham808 Jul 2, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karlnapf commented Aug 6, 2018

shubham808 commented Jun 29, 2018 •

edited

karlnapf Jun 30, 2018 •

edited

karlnapf Jul 2, 2018 •

edited

karlnapf Jul 2, 2018 •

edited

shubham808 Jul 2, 2018 •

edited