Use iterative machine mixin in AveragedPerceptron #4551

ghost · 2019-03-02T10:20:49Z

@shubham808 This PR addresses the project 'inside the black box II' and issue #4488.
I still need to write some unit tests, but I would like to get confirmation that this is the right way, since I'm still new to this project.

karlnapf

Hi

Thanks a lot for the PR! Much appreciated!
I made some initial comments. It probably needs a few more iterations or minor changes, @shubham808 will probably have more comments.

The tests fail because the class is not recognised by class list anymore. Check the formatting of the class definition of perceptron and make it like this here, then the error should go away (there is some python code that scans the codebase for all classes and it expects a certain formatting iirc

src/shogun/classifier/AveragedPerceptron.cpp

src/shogun/classifier/AveragedPerceptron.h

shubham808 · 2019-03-02T17:28:11Z

src/shogun/classifier/AveragedPerceptron.cpp

-	max_iter = 1000;
-	learn_rate = 0.1;
-	SG_ADD(&max_iter, "max_iter", "Maximum number of iterations.", ParameterProperties::HYPER);
+	set_max_iter(1000);


yes this is better

shubham808 · 2019-03-02T17:32:54Z

good first pr :)
a unit test is also necessary since we need to make sure what we did actually works, see consistency tests here

src/shogun/classifier/AveragedPerceptron.h

ghost · 2019-03-03T04:24:08Z

@shubham808 consistency_training_tests has that test that makes sure that two models: one with max_iterations = 1 and the other stopped after an iteration, yield the same results. This isn't true here, since the one with max_iterations=1 has different weights because end_training() method is called after training is finished, which changes the weights to the final form (performs averaging).
For this to work, end_trainig() has to be called when training is stopped for whatever reason. But this will require us to copy intermediate weights, so that training can be resumed with the intermediate weights instead of the averaged ones.
Edit: My suggestion won't work since end_training() is also used to free used resources by other algorithms. So maybe something like pause_training()?
Edit2: In this implementation of AveragedPerceptron, weights are summed across iterations, then averaged. Maybe this can happen online: mean_{n+1} = mean_{n} + (x - mean_{n}) / (n + 1)

karlnapf

See comments

karlnapf · 2019-03-03T07:52:26Z

src/shogun/classifier/AveragedPerceptron.h

-		int32_t max_iter;
+
+	private:
+		float64_t tmp_bias;


Could we maybe name those two a bit better?

Current name is cached_bias and cached_w, which is still bad I think. I will think of something better.

what about just saying what it is? w_averaged etc?

tests/unit/classifier/AveragedPeceptron_unittest.cc

src/shogun/classifier/AveragedPerceptron.h

tests/unit/classifier/AveragedPeceptron_unittest.cc

karlnapf · 2019-03-03T10:30:27Z

@shubham808 consistency_training_tests has that test that makes sure that two models: one with max_iterations = 1 and the other stopped after an iteration, yield the same results. This isn't true here, since the one with max_iterations=1 has different weights because end_training() method is called after training is finished, which changes the weights to the final form (performs averaging).
For this to work, end_trainig() has to be called when training is stopped for whatever reason. But this will require us to copy intermediate weights, so that training can be resumed with the intermediate weights instead of the averaged ones.
Edit: My suggestion won't work since end_training() is also used to free used resources by other algorithms. So maybe something like pause_training()?
Edit2: In this implementation of AveragedPerceptron, weights are summed across iterations, then averaged. Maybe this can happen online: mean_{n+1} = mean_{n} + (x - mean_{n}) / (n + 1)

We had this issue before. I dont remember how we exactly solved it. A running average seems to work here and seems like a very good idea. Make sure to pick a numerically stable one (might require some googling). Could you add a way to update a mean vector/scalar in linalg in a separate PR? Then you can rebase this PR here to use it. This would be better than just adding the explicit code in here and then it could be used in other algorithms (LARS comes to mind where it is done explicitly as well)

tests/unit/classifier/AveragedPeceptron_unittest.cc

karlnapf · 2019-03-09T14:46:21Z

src/shogun/classifier/AveragedPerceptron.cpp

+	cached_w = SGVector<float64_t>(num_feat);
+	// start with uniform w, bias=0, tmp_bias=0
+	bias = 0;
+	cached_bias = 0;


actually this name is fine imo

karlnapf · 2019-03-09T14:47:05Z

src/shogun/classifier/AveragedPerceptron.cpp

 		}
-		iter++;
-		pb.print_progress();
+		linalg::update_mean(w, cached_w, num_prev_weights);


sweet, much nicer to read!

src/shogun/classifier/Perceptron.cpp

karlnapf · 2019-03-09T14:49:40Z

tests/unit/utils/SGObjectIterator.h

+		return std::string(XSTRING__(ptype));                                  \
+	}
+
+SG_PRIMITIVE_TYPE(bool, PT_BOOL);


@gf712 do we have a central map for these?

all the supported types should go here

shogun/src/shogun/lib/sg_types.h

Line 41 in 801bdba

using sg_feature_types = Types<

and all the any supported types are here

shogun/src/shogun/lib/type_case.h

Line 118 in 801bdba

SG_ADD_PRIMITIVE_TYPE(bool, TYPE::T_BOOL)

@gf712 DataType.h seems to be obsolete, but I can't use sg_types and type_case yet since the class_list script (which has the create function) uses the old code.
Btw, the implementation of these two files is very interesting!

tests/unit/utils/SGObjectIterator.h

karlnapf · 2019-03-09T14:51:17Z

tests/unit/utils/SGObjectIterator.h

@@ -0,0 +1,164 @@
+#ifndef __SG_OBJECT_ITERATOR_H__


good idea moving this into a new file!

karlnapf · 2019-03-09T14:53:55Z

tests/unit/machine/IterativeMachine_unittest.cc

+
+using namespace shogun;
+
+std::set<std::string> sg_linear_machines = {"Perceptron", "AveragedPerceptron",


ok! This is definitely better than before. I have a question: If a test fails, is it easy to see which class is the offender?

Filtering the tests obviously is not possible via this though. Compile time is an advantage to the typed version of this we had before (we define a bunch of types and then essentially template the test). This then allows for filtering. Maybe in a follow up PR, we can do something about that. I think the current blocker was that it is hard to find all subclasses automatically. However, if we simply define a type list (as you have done here with strings), that is also acceptable for now.

@gf712 might also be interesting for you

It should be easy using SCOPED_TRACE.
Hard coding the linear machines may not be so scaleable. Can't the class_list script be extended to detect hierarchies?
Sorry I don't get your point about filtering, and why templated tests can be better.

Cool add the scoping!
Yes we can do this extension, but for now the explicit list is fine.
Compiled types can be helpful but for now this is good!

karlnapf · 2019-03-09T14:54:35Z

tests/unit/machine/IterativeMachine_unittest.cc

+		machine_stop->train(features);
+
+		machine_iters->set_labels(labels);
+		machine_iters->put<int32_t>("max_iterations", max_iters);


there should be yet a typed setter for this or?

since this helps to make the tests for reliable (no runtime error when setting things as here), we can keep the setter (even though it will be hidden from users in the future)

karlnapf

I like this update! :) Thanks!

So two things.

Can we somehow set the continue features in IterativeMachine?
a compiled test for every IterativeMachine might be easier to handle debugging wise. Something to look into? We can keep the runtime version for now
See some minor inline comments

shubham808 · 2019-03-09T16:31:06Z

@theartful Nice clean up for the tests, CI error seems unrelated
about the setting of continue features maybe we could try doing that here ?

ghost · 2019-03-10T06:09:05Z

Unrelated, but for some reason the LibLinearRegression test on MacOS passed :"D

karlnapf · 2019-03-10T08:51:41Z

Cool ci passed so this is almost good to go. I’ll do one more detailed read through later and then we can merge

shubham808 · 2019-03-10T10:18:57Z

@theartful lets check for memory leaks of the new test and then this is good to go from my side 👍

vigsterkr · 2019-03-15T11:43:35Z

tests/unit/utils/SGObjectIterator.h

+#include <string>
+#include <shogun/base/class_list.h>
+
+using namespace shogun;


@theartful plz never do this in a header file...

my bad didn't see it

ouch! sorry for that

@vigsterkr

shogun/tests/unit/utils/SGObjectIterator.h

Line 9 in 79fa98a

namespace

shouldn't it be namespace shogun?

* Add function to set features in iterative machine * Use iterative machine mixin in AveragedPerceptron * Add generic iterative machine test * Move m_continue_features assignment to IterativeMachine

karlnapf requested changes Mar 2, 2019

View reviewed changes

shubham808 reviewed Mar 2, 2019

View reviewed changes

src/shogun/classifier/AveragedPerceptron.h Show resolved Hide resolved

karlnapf requested changes Mar 3, 2019

View reviewed changes

gf712 reviewed Mar 4, 2019

View reviewed changes

tests/unit/classifier/AveragedPeceptron_unittest.cc Outdated Show resolved Hide resolved

theartful added 2 commits March 8, 2019 09:58

Add function to set features in iterative machine

ec9837c

Use iterative machine mixin in AveragedPerceptron

046e9e3

karlnapf reviewed Mar 9, 2019

View reviewed changes

src/shogun/classifier/Perceptron.cpp Outdated Show resolved Hide resolved

karlnapf reviewed Mar 9, 2019

View reviewed changes

tests/unit/utils/SGObjectIterator.h Outdated Show resolved Hide resolved

karlnapf reviewed Mar 9, 2019

View reviewed changes

tests/unit/utils/SGObjectIterator.h

@@ -0,0 +1,164 @@

#ifndef __SG_OBJECT_ITERATOR_H__

Copy link

Member

karlnapf Mar 9, 2019

Choose a reason for hiding this comment

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

good idea moving this into a new file!

karlnapf reviewed Mar 9, 2019

View reviewed changes

karlnapf requested changes Mar 9, 2019

View reviewed changes

theartful added 2 commits March 10, 2019 07:36

Add generic iterative machine test

4a2abdf

Move m_continue_features assignment to IterativeMachine

9a17fd5

karlnapf approved these changes Mar 10, 2019

View reviewed changes

karlnapf merged commit 5c45bbd into shogun-toolbox:develop Mar 11, 2019

vigsterkr reviewed Mar 15, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use iterative machine mixin in AveragedPerceptron #4551

Use iterative machine mixin in AveragedPerceptron #4551

ghost commented Mar 2, 2019 •

edited by ghost

karlnapf left a comment

shubham808 Mar 2, 2019

shubham808 commented Mar 2, 2019

ghost commented Mar 3, 2019 •

edited by ghost

karlnapf left a comment

karlnapf Mar 3, 2019

ghost Mar 9, 2019

karlnapf Mar 9, 2019

karlnapf commented Mar 3, 2019

karlnapf Mar 9, 2019

karlnapf Mar 9, 2019

karlnapf Mar 9, 2019

gf712 Mar 10, 2019

ghost Mar 10, 2019 •

edited by ghost

karlnapf Mar 9, 2019

karlnapf Mar 9, 2019

ghost Mar 10, 2019 •

edited by ghost

karlnapf Mar 10, 2019

karlnapf Mar 9, 2019

karlnapf Mar 9, 2019

karlnapf left a comment

shubham808 commented Mar 9, 2019

ghost commented Mar 10, 2019

karlnapf commented Mar 10, 2019

shubham808 commented Mar 10, 2019

vigsterkr Mar 15, 2019

karlnapf Mar 15, 2019

ghost Mar 15, 2019

ghost Mar 19, 2019


		using namespace shogun;

		std::set<std::string> sg_linear_machines = {"Perceptron", "AveragedPerceptron",

Use iterative machine mixin in AveragedPerceptron #4551

Use iterative machine mixin in AveragedPerceptron #4551

Conversation

ghost commented Mar 2, 2019 • edited by ghost

karlnapf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shubham808 commented Mar 2, 2019

ghost commented Mar 3, 2019 • edited by ghost

karlnapf left a 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 Mar 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

ghost Mar 10, 2019 • edited by ghost

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost Mar 10, 2019 • edited by ghost

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

shubham808 commented Mar 9, 2019

ghost commented Mar 10, 2019

karlnapf commented Mar 10, 2019

shubham808 commented Mar 10, 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

ghost commented Mar 2, 2019 •

edited by ghost

ghost commented Mar 3, 2019 •

edited by ghost

ghost Mar 10, 2019 •

edited by ghost

ghost Mar 10, 2019 •

edited by ghost