Refactor NearestCentroid class #5053

LiuYuHui · 2020-06-03T01:40:58Z

Add fit/predict to Machine class
Refactor NearestCentroid to be stateless
Add NearestCentroid unittest

src/shogun/classifier/NearestCentroid.cpp

vigsterkr

some food for thought

vigsterkr · 2020-06-03T06:11:02Z

src/shogun/classifier/NearestCentroid.cpp

 	}

+	bool NearestCentroid::train_machine(std::shared_ptr<Features> data)
+	{
+		error("the train_machine have been deprecated, please use fit instead");


uhoh :) the deprecation should mean that it's still functional :)
so either use a warning and still be functional (but i guess that's not possible coz of not having labels) so in this case i would just drop the whole thing :)
if Machine::train_machine is a pure virtual function, then maybe we should just create there an implementation with this error....

src/shogun/machine/Machine.h

vigsterkr · 2020-06-03T06:12:09Z

src/shogun/machine/Machine.h

+		 *
+		 * @return The predicted Label for each sample in X .
+		 */
+		virtual std::shared_ptr<Labels> predict(std::shared_ptr<Features> X)


Suggested change

virtual std::shared_ptr<Labels> predict(std::shared_ptr<Features> X)

virtual std::shared_ptr<Labels> predict(const std::shared_ptr<Features>& X)

predict should be const right?

src/shogun/classifier/NearestCentroid.cpp

gf712 · 2020-06-03T06:54:30Z

src/shogun/classifier/NearestCentroid.cpp

-			if(total>1)
-				scale[i] = 1.0/((float64_t)(total-1));
+			if (total > 0)
+				scale[i] = 1.0 / (float64_t)(total);


Suggested change

scale[i] = 1.0 / (float64_t)(total);

scale[i] = 1.0 / static_cast<float64_t>(total);

Doesn't change anything but it is more explicit

gf712 · 2020-06-03T06:59:09Z

src/shogun/classifier/NearestCentroid.cpp

 		auto iter_labels = multiclass_labels->get_labels().begin();
+		std::map<int64_t, int64_t> label_to_index;


why not unordered_map and then reserve num_classes?

this labels to index, this is a problem that re-occurs in other places, so I'd not solve it here.
Actually, we have some mechanisms in place that convert labels internally (code is e.g. in multiclass labels).

@karlnapf didn't you start a PR that does this mapping?

yes, there is something merged even.
But we need to re-think/redesign parts of it as it had issues ...

gf712 · 2020-06-03T07:03:08Z

src/shogun/classifier/NearestCentroid.cpp

 		for (const auto& current : DotIterator(dense_data))
 		{
 			const auto current_class = static_cast<int32_t>(*(iter_labels++));
-			auto target = centroids.get_column(current_class);
+			auto curr_index = index;
+			if (label_to_index.find(current_class) == label_to_index.end())


You could rewrite this in order to reuse the iterator in the else branch. I am not sure if the compiler will do this for you and you might get some performance considering this is a inner loop.

gf712 · 2020-06-03T07:05:05Z

src/shogun/classifier/NearestCentroid.cpp


 		int32_t num_classes = multiclass_labels->get_num_classes();
 		int32_t num_feats = dense_data->get_num_features();

 		SGMatrix<float64_t> centroids(num_feats, num_classes);
 		SGVector<int64_t> num_per_class(num_classes);
-
+		linalg::zero(centroids);
+		linalg::zero(num_per_class);


Doesn't the constructor memset to 0?

gf712 · 2020-06-03T07:06:37Z

This is great! :)

gf712 · 2020-06-03T07:51:36Z

src/shogun/classifier/NearestCentroid.cpp

-		auto multiclass_labels = m_labels->as<MulticlassLabels>();
-		auto dense_data = data->as<DenseFeatures<float64_t>>();
+		auto multiclass_labels = lab->as<MulticlassLabels>();
+		auto dense_data = feat->as<DenseFeatures<float64_t>>();


these could be const right?

gf712 · 2020-06-03T07:51:51Z

src/shogun/classifier/NearestCentroid.cpp

-		auto multiclass_labels = m_labels->as<MulticlassLabels>();
-		auto dense_data = data->as<DenseFeatures<float64_t>>();
+		auto multiclass_labels = lab->as<MulticlassLabels>();
+		auto dense_data = feat->as<DenseFeatures<float64_t>>();

 		int32_t num_classes = multiclass_labels->get_num_classes();
 		int32_t num_feats = dense_data->get_num_features();


const I think?

gf712 · 2020-06-03T07:53:25Z

src/shogun/classifier/NearestCentroid.h

-	 * @return whether training was successful
+	 * @return The predicted Label for each sample in X .
+	 */
+	virtual std::shared_ptr<Labels> predict(std::shared_ptr<Features> X);


Suggested change

virtual std::shared_ptr<Labels> predict(std::shared_ptr<Features> X);

std::shared_ptr<Labels> predict(std::shared_ptr<Features> X) override;

gf712 · 2020-06-03T07:54:09Z

src/shogun/classifier/NearestCentroid.h

+	 */
+
+	virtual std::shared_ptr<Machine>
+	fit(std::shared_ptr<Features> feat, std::shared_ptr<Labels> lab);


Suggested change

fit(std::shared_ptr<Features> feat, std::shared_ptr<Labels> lab);

fit(std::shared_ptr<Features> feat, std::shared_ptr<Labels> lab) override;

karlnapf · 2020-06-03T11:22:19Z

src/shogun/classifier/NearestCentroid.cpp

-			data = distance->get_lhs();
-		}
+		// ASSERT(distance)
+		require(distance, "distance not set");


nitpick minor: Capital D

karlnapf · 2020-06-03T11:23:08Z

src/shogun/classifier/NearestCentroid.cpp

-		}
+		// ASSERT(distance)
+		require(distance, "distance not set");
+		require(


@vigsterkr @gf712 could we pls move this check into the base class? It is repeated 100 times in all sorts of different styles ...

agreed, the labels and features base classes implement get_num_labels and get_num_vectors, so it makes sense to have the check in Machine

karlnapf · 2020-06-03T11:25:54Z

src/shogun/classifier/NearestCentroid.cpp

+	std::shared_ptr<Labels>
+	NearestCentroid::predict(std::shared_ptr<Features> data)
+	{
+		require(data, "features not set");


@vigsterkr @gf712 while we are touching these things. Wouldnt it be better to do all these sort of input checks in the base class and then call a (virtual) method that can assume that

the pointers are not null

the number of examples are the same

so we don't have to have all this duplicate checks?

yup agreed, like train calls train_machine, predict does the checks and predict_labels is pure virtual

gf712 · 2020-06-04T13:21:35Z

tests/unit/classifier/NearestCentroid_unittest.cc

+{
+	SGMatrix<float64_t> X{{-10, -1}, {-2, -1}, {-3, -2},
+	                      {1, 1},    {2, 1},   {3, 2}};
+	SGVector<float64_t> y{1, 1, 1, 2, 2, 2};


SGVector<float64_t> y{0,0,0,1,1,1};

karlnapf · 2020-06-16T10:02:39Z

might to rebase/force push

karlnapf · 2020-06-17T09:38:31Z

src/shogun/classifier/NearestCentroid.cpp

@@ -34,25 +32,16 @@ namespace shogun{

 	void NearestCentroid::init()
 	{
-		m_shrinking=0;
-		m_is_trained=false;
+


karlnapf

lgtm

gf712 · 2020-06-18T11:49:21Z

examples/meta/src/multiclass/k_nearest_neighbours.sg.in

@@ -16,7 +16,7 @@ Distance d = create_distance("EuclideanDistance")

 #![create_instance]
 int k = 3
-Machine knn = create_machine("KNN", k=k, distance=d, labels=labels_train)
+KNN knn(k, d)


why not Machine knn = create_machine("KNN", k=k, distance=d)?

gf712 · 2020-06-18T21:09:14Z

doc/ipython-notebooks/classification/Classification.ipynb

@@ -552,7 +552,7 @@
    "plt.figure(figsize=(15,5))\n",
    "plt.subplot(121)\n",
    "plt.title(\"Nearest Neighbors - Linear Features\")\n",
-    "plot_model(plt,knn_linear,feats_linear,labels_linear,fading=False)\n",
+    "plot_model(plt,knn_linear,feats_linear,labels_linear,fading=True)\n",


gf712 · 2020-07-30T11:01:56Z

I think you changed the data commit?

* add nonparametric machine * fix notebooks

gf712 · 2020-07-31T06:40:48Z

Can merge this as the CI error seems to be unrelated

* Add NonParametricMachine class (#5055) * add nonparametric machine * fix notebooks * Refactor NearestCentroid class

LiuYuHui commented Jun 3, 2020

View reviewed changes

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

LiuYuHui force-pushed the refactor-class-stateless branch from 2c5a5b4 to 2e6f793 Compare June 3, 2020 02:15

vigsterkr requested changes Jun 3, 2020

View reviewed changes

gf712 reviewed Jun 3, 2020

View reviewed changes

karlnapf reviewed Jun 3, 2020

View reviewed changes

gf712 reviewed Jun 4, 2020

View reviewed changes

LiuYuHui changed the base branch from develop to feature/machine_refactor June 16, 2020 00:47

LiuYuHui force-pushed the refactor-class-stateless branch from 2e6f793 to 7cfb209 Compare June 16, 2020 03:07

LiuYuHui force-pushed the refactor-class-stateless branch from 7cfb209 to ea3da01 Compare June 17, 2020 02:51

LiuYuHui changed the title ~~Refactor NearestCentroid to be stateless~~ Refactor NearestCentroid class Jun 17, 2020

karlnapf reviewed Jun 17, 2020

View reviewed changes

gf712 force-pushed the feature/machine_refactor branch 5 times, most recently from 9fae5e2 to b9d5bfa Compare June 18, 2020 10:09

LiuYuHui force-pushed the refactor-class-stateless branch from 5c70702 to cec247b Compare June 18, 2020 11:37

gf712 reviewed Jun 18, 2020

View reviewed changes

LiuYuHui force-pushed the refactor-class-stateless branch from ba12148 to af2a0fc Compare June 22, 2020 01:42

gf712 force-pushed the feature/machine_refactor branch from 5a866f9 to 6d77d19 Compare July 6, 2020 08:23

gf712 force-pushed the feature/machine_refactor branch from e2673fc to 97f1d5d Compare July 28, 2020 16:45

LiuYuHui force-pushed the refactor-class-stateless branch from af2a0fc to fb4e506 Compare July 29, 2020 09:02

gf712 force-pushed the feature/machine_refactor branch 2 times, most recently from 72046cb to aa741be Compare July 29, 2020 13:43

LiuYuHui force-pushed the refactor-class-stateless branch from fb4e506 to 3bb0ffe Compare July 30, 2020 02:19

gf712 force-pushed the feature/machine_refactor branch 3 times, most recently from aa741be to 7a37af4 Compare July 30, 2020 08:56

LiuYuHui and others added 4 commits July 30, 2020 19:15

Add NonParametricMachine class (shogun-toolbox#5055)

c569eb3

* add nonparametric machine * fix notebooks

Refactor NearestCentroid class

92719b6

remove function that not needed

5c7ed71

fix errors

aa414d5

LiuYuHui force-pushed the refactor-class-stateless branch from c61b683 to aa414d5 Compare July 30, 2020 11:20

LiuYuHui added 2 commits July 30, 2020 19:22

fix errors

590684d

fix jupyter

3f8c07e

gf712 merged commit afbdeac into shogun-toolbox:feature/machine_refactor Jul 31, 2020

gf712 pushed a commit that referenced this pull request Dec 8, 2020

Refactor NearestCentroid class (#5053)

2b093cf

* Add NonParametricMachine class (#5055) * add nonparametric machine * fix notebooks * Refactor NearestCentroid class

gf712 pushed a commit that referenced this pull request Dec 8, 2020

Refactor NearestCentroid class (#5053)

fdf8555

* Add NonParametricMachine class (#5055) * add nonparametric machine * fix notebooks * Refactor NearestCentroid class

gf712 pushed a commit that referenced this pull request Dec 8, 2020

Refactor NearestCentroid class (#5053)

85bfb7f

* Add NonParametricMachine class (#5055) * add nonparametric machine * fix notebooks * Refactor NearestCentroid class

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor NearestCentroid class #5053

Refactor NearestCentroid class #5053

LiuYuHui commented Jun 3, 2020

vigsterkr left a comment

vigsterkr Jun 3, 2020

vigsterkr Jun 3, 2020

vigsterkr Jun 3, 2020

gf712 Jun 3, 2020

gf712 Jun 3, 2020

gf712 Jun 3, 2020

karlnapf Jun 3, 2020

gf712 Jun 3, 2020

karlnapf Jun 4, 2020

gf712 Jun 3, 2020 •

edited

Loading

gf712 Jun 3, 2020

gf712 commented Jun 3, 2020

gf712 Jun 3, 2020

gf712 Jun 3, 2020

gf712 Jun 3, 2020

gf712 Jun 3, 2020

karlnapf Jun 3, 2020

karlnapf Jun 3, 2020

gf712 Jun 3, 2020 •

edited

Loading

karlnapf Jun 3, 2020

gf712 Jun 3, 2020

gf712 Jun 4, 2020

karlnapf commented Jun 16, 2020

karlnapf Jun 17, 2020

karlnapf left a comment

gf712 Jun 18, 2020

gf712 Jun 18, 2020

gf712 commented Jul 30, 2020

gf712 commented Jul 31, 2020

	virtual std::shared_ptr<Labels> predict(std::shared_ptr<Features> X)
	virtual std::shared_ptr<Labels> predict(const std::shared_ptr<Features>& X)

	scale[i] = 1.0 / (float64_t)(total);
	scale[i] = 1.0 / static_cast<float64_t>(total);

		auto iter_labels = multiclass_labels->get_labels().begin();
		std::map<int64_t, int64_t> label_to_index;

	virtual std::shared_ptr<Labels> predict(std::shared_ptr<Features> X);
	std::shared_ptr<Labels> predict(std::shared_ptr<Features> X) override;

	fit(std::shared_ptr<Features> feat, std::shared_ptr<Labels> lab);
	fit(std::shared_ptr<Features> feat, std::shared_ptr<Labels> lab) override;

Refactor NearestCentroid class #5053

Refactor NearestCentroid class #5053

Conversation

LiuYuHui commented Jun 3, 2020

vigsterkr 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

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

gf712 Jun 3, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gf712 commented Jun 3, 2020

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

gf712 Jun 3, 2020 • edited Loading

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 Jun 16, 2020

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

Choose a reason for hiding this comment

gf712 commented Jul 30, 2020

gf712 commented Jul 31, 2020

gf712 Jun 3, 2020 •

edited

Loading

gf712 Jun 3, 2020 •

edited

Loading