Refactor linear machine api #4319

vinx13 · 2018-05-31T11:02:02Z

[WIP]
Adapt machine api to fit + predict
This need some heavy refactor, so let's start from linear machines and iterate a few times.
The idea is to keep both old and new apis working and then remove old apis gradually.
train_machine(CFeatures*, CLabels*) is added to machines that need labels. The old method, train_machine(CFeatures*) will redirect to the new api and pass m_labels as the labels argument. In this way, the old api (set_labels + train(CFeatures*)) still works.

Roadmap

Add void fit(CFeatures*), void fit(CFeatures*, CLabels*) to CMachine
Redirect bool train_machine(CFeatures*) to void train_machine(CFeatures*, CLabels*) in LinearMachine
Calling new api in unittests
Move api redirection to LinearMachine as a base class method after the new api works for all LinearMachine subclasses
Make bool train_machine(CFeatures*) and bool train_machine_templated(CFeatures*) return void (The latter one should be easier as it is added last year and we haven't used in many places)

Known issues for moving to const methods:

ref counting
labels factory (e.g. regression_labels) doesn't accept const args

karlnapf · 2018-05-31T12:28:27Z

src/shogun/classifier/AveragedPerceptron.cpp

@@ -40,59 +32,51 @@ void CAveragedPerceptron::init()
 	SG_ADD(&learn_rate, "learn_rate", "Learning rate.", MS_AVAILABLE);
 }

-bool CAveragedPerceptron::train_machine(CFeatures* data)
+void CAveragedPerceptron::train_machine(CFeatures* features, CLabels* labels)


dont we want this to be const parameters?

of course. unfortunately we can't use const for the time being because there are many non-const methods that should be const logically, e.g. get_feature_matrix

I see, sure
(though get_feature_matrix can be made const without problem)

karlnapf · 2018-05-31T12:29:13Z

src/shogun/classifier/AveragedPerceptron.cpp

 		{
-			output[i] = features->dense_dot(i, w.vector, w.vlen) + bias;
+			output[i] = dot_features->dense_dot(i, w.vector, w.vlen) + bias;


i think this could be done via DotIterator

karlnapf · 2018-05-31T12:29:45Z

src/shogun/classifier/AveragedPerceptron.cpp

 	bias=tmp_bias/(num_vec*iter);

-	SG_FREE(output);
-	SG_FREE(tmp_w);
-
 	set_w(w);


i would prefer to remove this and udpate the state vector itself inside the main loop.
@shubham808 can elaborate as he is doing similar stuff for the perceptron

long time ago this comment. But basically we want to start updating model states inside the loops for iterative machines

@shubham808 comments?

@karlnapf I think set_w has been already called inside the iteration function?

你是对的，对不起

karlnapf · 2018-05-31T12:30:47Z

src/shogun/classifier/Perceptron.cpp

 {
-	ASSERT(m_labels)
+	if (!features->has_property(FP_DOT))


why not as? would be a bit cleaner. Training is expensive, so the additional costs shouldnt matter or?

we can provide more informative error message here, but as is as good

karlnapf · 2018-05-31T12:32:48Z

src/shogun/machine/Machine.cpp

@@ -66,6 +66,23 @@ bool CMachine::train(CFeatures* data)
 	return result;
 }

+void CMachine::fit(CFeatures* features)
+{
+	REQUIRE(train(features), "Failed to fit machine %s\n", get_name());


maybe we should throw an exception rather than this boolean stuff?

require failed -> sg_io->msg(MSG_ERROR, "blabla") -> https://github.com/shogun-toolbox/shogun/blob/develop/src/shogun/io/SGIO.cpp#L125

Sure but I mean, shouldn't the train method itself just throw an exception?
We don't get any context information here. If train would throw an exception, we could catch it here and then say "training failed for reason X". No?

i'm going to just throw exceptions in fit. but for now train returns boolean, i keep this so that it is consistent that fit always throw exceptions when failure.

karlnapf · 2018-12-11T18:05:49Z

Hi!
Shall we pick this up again?

vinx13 · 2018-12-12T12:41:24Z

@karlnapf I found there are too many things involved here. When you change the Machine base class, almost everything need to be updated. I would suggest start some small refactor first, such as Distance or Kernel. We may further discuss on irc.

karlnapf · 2018-12-18T11:55:16Z

src/shogun/classifier/svm/LibLinear.cpp


-	ASSERT(m_labels)
-	init_linear_term();
+void CLibLinear::train_machine(CFeatures* features, CLabels* labels)


const possible?

There are many non-const methods algorithm internal methods. We can either use non-const arguments here, or use const_cast to drop const in internal methods

@karlnapf One problem ref counting is IterativeMachine. We need to increase ref count of features and labels in init_model

ah crap yes, the ref counting is non-const....
ok so we need to postpone the const making for now until we have another way to do the reference counting

karlnapf · 2018-12-18T11:56:03Z

Nice! Let me know when we should have a look at this...

karlnapf · 2018-12-20T10:29:16Z

I like the idea of porting old methods to the new nicer api using const casts. Although we might run into see trouble doing that.... @lisitsyn @iglesias @vigsterkr ?

karlnapf · 2019-01-03T17:43:46Z

src/shogun/classifier/LPBoost.cpp

+
+virtual void CLPBoost::train_machine(CFeatures* features, CLabels* labels);
+{
+	ASSERT(labels)


I think handling those asserts happens (should happen?) in the base class?

Agree, it should be in the base class. I will check if it is possible for now.

karlnapf · 2019-01-03T17:44:43Z

src/shogun/base/SGObject.h

@@ -567,6 +567,20 @@ class CSGObject
 			demangled_type<T>().c_str());
 		return nullptr;
 	}
+
+	template <class T> const T* as() const


you are using this below I assume when changing the signatures of some methods?

This is useful for const pointers (const CFeatures*). Although we decided to move to const methods later, I leave it here for possible future usage.

karlnapf · 2019-01-03T17:45:04Z

Cool some progress :) Let us know how you are getting on here

vinx13 · 2019-01-07T17:28:17Z

src/shogun/regression/LeastSquaresRegression.cpp

@@ -24,4 +18,3 @@ CLeastSquaresRegression::CLeastSquaresRegression(CDenseFeatures<float64_t>* data
 : CLinearRidgeRegression(0, data, lab)


@karlnapf Got some linking issue

Undefined symbols for architecture x86_64: 2019-01-07T16:59:36.8868140Z "bool shogun::CLinearRidgeRegression::train_machine_templated<double>(shogun::CDenseFeatures<double> const*)", referenced from: 2019-01-07T16:59:36.8911800Z shogun::CDenseRealDispatch<shogun::CLinearRidgeRegression, shogun::CLinearMachine>::train_dense(shogun::CFeatures*) in LeastSquaresRegression.cpp.o 2019-01-07T16:59:36.9066190Z "bool shogun::CLinearRidgeRegression::train_machine_templated<long double>(shogun::CDenseFeatures<long double> const*)", referenced from: 2019-01-07T16:59:36.9110490Z shogun::CDenseRealDispatch<shogun::CLinearRidgeRegression, shogun::CLinearMachine>::train_dense(shogun::CFeatures*) in LeastSquaresRegression.cpp.o 2019-01-07T16:59:36.9263540Z "bool shogun::CLinearRidgeRegression::train_machine_templated<float>(shogun::CDenseFeatures<float> const*)", referenced from: 2019-01-07T16:59:36.9306660Z shogun::CDenseRealDispatch<shogun::CLinearRidgeRegression, shogun::CLinearMachine>::train_dense(shogun::CFeatures*) in LeastSquaresRegression.cpp.o

CLinearRidgeRegression::train_machine_templated is a template method, its definition is in LinearRidgeRegression.cpp and is not visible in this cpp file.

@Saurabh7 this would be something for you to check/figure out? I think we have seen and solved something similar before. Unless you have ideas @vinx13 ?

@karlnapf I only saw this error on CI. I cannot reproduce this locally with docker image.

Yes I remember this issue somehow with the same reproducibility issues. Maybe @Saurabh7 remembers

I assume this works now?

yes, we need to explicit instantiate templates in cpp files to make it linkable

Looks like this is solved.

Very Interesting PR btw ! So we are planning to change all APIs to .fit , .predict ?

Sorry I actually meant to ping @shubham808 ;)
But yes @Saurabh7 we are intending to do that

karlnapf · 2019-01-18T14:55:48Z

src/shogun/base/SGObject.h

+
+		SG_SERROR(
+			"Object of type %s cannot be converted to type %s.\n",
+			demangled_type<std::remove_pointer_t<decltype(this)>>().c_str(),


btw can we re-use code from the non-const version in here? Like one calls the other?

stale · 2020-02-26T15:52:10Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

gf712 · 2020-02-26T16:44:34Z

keeping this alive as I think this is still the direction we want to go in

stale · 2020-08-24T17:19:00Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

karlnapf · 2020-08-28T14:31:58Z

bump

stale · 2021-02-25T20:06:48Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

stale · 2021-03-04T22:24:32Z

This issue is now being closed due to a lack of activity. Feel free to reopen it.

karlnapf reviewed May 31, 2018

View reviewed changes

vigsterkr force-pushed the feature/transformers branch 2 times, most recently from b7d2ff9 to f211b65 Compare June 8, 2018 14:15

vigsterkr force-pushed the feature/transformers branch from f211b65 to 0fa4faa Compare June 11, 2018 18:30

vigsterkr force-pushed the feature/transformers branch from 550639e to af8af72 Compare June 28, 2018 10:49

vigsterkr force-pushed the feature/transformers branch 3 times, most recently from a241ab1 to dbfd69e Compare July 12, 2018 16:02

vinx13 changed the base branch from feature/transformers to develop December 12, 2018 15:45

vinx13 force-pushed the feature/machine_api branch 3 times, most recently from a2078d9 to fb4930c Compare December 15, 2018 06:30

karlnapf reviewed Dec 18, 2018

View reviewed changes

karlnapf reviewed Jan 3, 2019

View reviewed changes

vinx13 force-pushed the feature/machine_api branch 2 times, most recently from eefbcf8 to 7caeb67 Compare January 7, 2019 16:43

vinx13 commented Jan 7, 2019

View reviewed changes

vinx13 force-pushed the feature/machine_api branch 2 times, most recently from 779b11a to 29d3bb6 Compare January 18, 2019 13:08

karlnapf reviewed Jan 18, 2019

View reviewed changes

vinx13 added 14 commits June 9, 2019 19:49

NewtonSVM

7e357f0

LibLinearRegression

1fd9dc2

SGDQN

114b3fa

DomainAdaptationSVMLinear

f090443

LibLinearMTL

afc679d

LeastAngleRegression

32cc50d

Make DenseLabels methods const

ff0d01b

LinearRidgeRegression

32211f2

Remove unused LAPACK import in LeastSquaresRegression

25d5751

Make train_machine_templated return void

f4ccedb

Use new api in unittests

6d18836

Clean up CRTP dispatcher

8fa12d5

Implement CLinearMachine::fit

60c96d8

Update

a8e8852

vinx13 force-pushed the feature/machine_api branch from 372b720 to 0cd2e63 Compare June 9, 2019 17:50

vinx13 mentioned this pull request Jun 9, 2019

Refactor linear machine api shogun-toolbox/shogun-gpl#11

Draft

Update GPL submodule

f58cade

vinx13 force-pushed the feature/machine_api branch from 0cd2e63 to f58cade Compare June 9, 2019 18:08

stale bot added the stale label Feb 26, 2020

stale bot removed the stale label Feb 26, 2020

stale bot added the stale label Aug 24, 2020

stale bot removed the stale label Aug 28, 2020

stale bot added the stale label Feb 25, 2021

stale bot closed this Mar 4, 2021

		@@ -24,4 +18,3 @@ CLeastSquaresRegression::CLeastSquaresRegression(CDenseFeatures<float64_t>* data
		: CLinearRidgeRegression(0, data, lab)

Refactor linear machine api #4319

Refactor linear machine api #4319

Conversation

vinx13 commented May 31, 2018 • edited

Roadmap

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

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

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 Dec 11, 2018

vinx13 commented Dec 12, 2018

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 Dec 18, 2018

karlnapf commented Dec 20, 2018

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 Jan 3, 2019

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

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

stale bot commented Feb 26, 2020

gf712 commented Feb 26, 2020

stale bot commented Aug 24, 2020

karlnapf commented Aug 28, 2020

stale bot commented Feb 25, 2021

stale bot commented Mar 4, 2021

vinx13 commented May 31, 2018 •

edited