Update the undocumented graphical python examples #4989

vinnik-dmitry07 · 2020-04-02T15:49:37Z

No description provided.

(cherry picked from commit 49194d0)

(cherry picked from commit e7d67d9)

(cherry picked from commit f8a0a05)

(cherry picked from commit 3da6820)

(cherry picked from commit 56fccb7)

(cherry picked from commit 39fbbb5)

(cherry picked from commit 1814ebd)

gf712 · 2020-04-02T15:52:28Z

examples/undocumented/python/graphical/mclda.py

 pos = np.array([x_pos, y_pos])
 neg = np.array([x_neg, y_neg])
-features = RealFeatures( np.array(np.concatenate([pos, neg], 1)) )
+
+features_ = features(np.array(np.concatenate([pos, neg], 1)))


@karlnapf maybe we should stop from shogun import * to avoid this kind of thing happening?

yes absolutely, we should use import shogun as sg and then sg.features (actually also in meta examples, Ill open an issue)

karlnapf · 2020-04-02T16:12:23Z

examples/undocumented/python/graphical/lda.py

@@ -1,32 +1,31 @@
-from pylab import figure,pcolor,scatter,contour,colorbar,show,subplot,plot,connect
+import matplotlib.pyplot as plt


I would put changes to python graphical example in a different PR than the C++ code below. EDIT. sorry I only now realised that those are cleanups....fine to have a single PR then

Also, note that graphical python examples are pretty much the bottom of our priority list, so let's get these changes in but let's not do more of these. Instead, I suggest you focus on sending some C++ code

I made these changes for vigsterkr can run this example, because there, and I guessed in some other places, was also the problem with const std::shared_ptr<DenseFeatures<float64_t>>&. I mean I did this, not because of the priorities. But now I have understood that it is a SWIG problem.

sure thing. the patch is welcome, let's get it in, just wanted to make sure you are on the same page as us

and it does make sense to improve the whole listing and make it ready for the new api, so then we don't have to touch it again anytime soon.

karlnapf · 2020-04-02T16:14:12Z

examples/undocumented/python/graphical/mclda.py

 import numpy as np
-import util
+from shogun import features


actually I just looked through the changes here, the only functional change is replacing RealFeatures with Features.
The file above has no functional changes as far as I can see.

karlnapf · 2020-04-02T16:16:23Z

examples/undocumented/python/graphical/mclda.py

@@ -1,63 +1,57 @@
-from shogun import RealFeatures
 from shogun import MulticlassLabels


could we have a import shogun as sg as per the issue I referenced?

(and remove all the other imports?)

karlnapf · 2020-04-02T16:17:14Z

examples/undocumented/python/graphical/util.py

@@ -1,10 +1,14 @@
 """ Utilities for matplotlib examples """

 import pylab
-from numpy import ones, array, double, meshgrid, reshape, linspace, \
-	concatenate, ravel, pi, sinc
+from numpy import ones, array, double, meshgrid, linspace, concatenate, ravel, pi, sinc


import numpy as np

karlnapf · 2020-04-02T16:18:58Z

src/shogun/distributions/KernelDensity.h

@@ -50,8 +50,8 @@ enum EEvaluationMode

 /** @brief This class implements the kernel density estimation technique. Kernel density estimation is a non-parametric
 * way to estimate an unknown pdf. The pdf at a query point given finite training samples is calculated using the
- * following formula : \\
- * \f$pdf(x')= \frac{1}{nh} \sum_{i=1}^n K(\frac{||x-x_i||}{h})\f$ \\
+ * following formula: \n


I think there are centred math tex commands in doxygen, so you dont have to do newlines by hand. See e.g. GaussianKernel.h

* \f[ * k({\bf x},{\bf x'})= exp(-\frac{||{\bf x}-{\bf x'}||^2}{\tau}) * \f]

karlnapf · 2020-04-02T16:19:41Z

src/shogun/multiclass/tree/KDTree.h

@@ -40,7 +40,7 @@ namespace shogun
 {

 /** @brief This class implements KD-Tree.
- * cf. http://www.autonlab.org/autonweb/14665/version/2/part/5/data/moore-tutorial.pdf
+ * cf. https://web.archive.org/web/20140327060654if_/http://www.autonlab.org:80/autonweb/14665/version/2/part/5/data/moore-tutorial.pdf


is this really a good idea? Why not put a reference as a citation (a different/better reference, or wikipedia)

I am not the author of code, so I decided to put what the author meant when was writing this comment

I saw that link from the archive works, I do not really investigate content. Sorry, will do better next time.

no worries at all. I just think dead-links should be removed rather than pulled out from the internet archive ;) wikepedia is probably fine here anyways.

karlnapf · 2020-04-02T16:20:13Z

examples/undocumented/python/graphical/lda.py

 from shogun import *
 import util

-util.set_title('LDA')


for the record. next time please make a smaller PR title. This one is way too epic ;)

karlnapf · 2020-04-02T16:21:11Z

examples/undocumented/python/graphical/lda.py

-features=util.get_realfeatures(pos, neg)
-lda=LDA(gamma, features, labels)
+labels = util.get_labels()
+features = util.get_realfeatures(pos, neg)


while we are at it get_real_features

karlnapf · 2020-04-02T16:22:02Z

examples/undocumented/python/graphical/lda.py

-lda=LDA(gamma, features, labels)
+labels = util.get_labels()
+features = util.get_realfeatures(pos, neg)
+lda = LDA(gamma, features, labels)


white we are at it. Could you port this to the new API usage, something in the lines of

lda = machine("LDA", gamma=gamma, labels=labels) lda.train(features)

karlnapf · 2020-04-02T16:22:30Z

examples/undocumented/python/graphical/mclda.py


 # train qda
-labels = MulticlassLabels( np.concatenate([np.zeros(N), np.ones(N)]) )
+labels = MulticlassLabels(np.concatenate([np.zeros(N), np.ones(N)]))


use sg.labels factory (new api)

karlnapf · 2020-04-02T16:22:37Z

examples/undocumented/python/graphical/mclda.py

 pos = np.array([x_pos, y_pos])
 neg = np.array([x_neg, y_neg])
-features = RealFeatures( np.array(np.concatenate([pos, neg], 1)) )
+
+features_ = features(np.array(np.concatenate([pos, neg], 1)))

 lda = MCLDA()


karlnapf · 2020-04-02T16:23:03Z

examples/undocumented/python/graphical/mclda.py

 pos = np.array([x_pos, y_pos])
 neg = np.array([x_neg, y_neg])
-features = RealFeatures( np.array(np.concatenate([pos, neg], 1)) )
+
+features_ = features(np.array(np.concatenate([pos, neg], 1)))

 lda = MCLDA()
 lda.set_labels(labels)


use put (or rather a kwarg when using the machine factory)

karlnapf · 2020-04-02T16:23:27Z

examples/undocumented/python/graphical/mclda.py


 x1 = np.linspace(x1_min, x1_max, size)
 x2 = np.linspace(x2_min, x2_max, size)

 x, y = np.meshgrid(x1, x2)

-dense = RealFeatures( np.array((np.ravel(x), np.ravel(y))) )
+dense = features(np.array((np.ravel(x), np.ravel(y))))
 dense_labels = lda.apply(dense).get_labels()


lda.apply(dense).get('labels')

(cherry picked from commit 2ad1148)

…ss_binary_classification.py. Remove RealFeatures. (cherry picked from commit 597ddd1)

…: reformat. (cherry picked from commit 06ec2b3)

(cherry picked from commit 96ef045)

karlnapf · 2020-04-03T17:52:41Z

examples/undocumented/python/graphical/classifier_gaussian_process_binary_classification.py

-    train_features = RealFeatures(X_train)
-    train_labels = BinaryLabels(y_train)
+    train_features = sg.features(X_train)
+    train_labels = sg.BinaryLabels(y_train)


sg.labels should work

karlnapf · 2020-04-03T17:53:13Z

examples/undocumented/python/graphical/classifier_gaussian_process_binary_classification.py


    # create zero mean function
-    mean = ZeroMean()
+    mean = sg.ZeroMean()


is there a factory for mean functions? I think so as we have a gp meta example

I did not find.

karlnapf · 2020-04-03T17:53:34Z

examples/undocumented/python/graphical/classifier_gaussian_process_binary_classification.py


    # create and train GP classifier, which uses Laplace approximation
-    gp = GaussianProcessClassification(inf)
+    gp = sg.GaussianProcessClassification(inf)


all those should have a factory, meta examples should guide you

How to run this method from the machine? (get_probabilities)

just checked, it is not possible currently, and we need to refactor this stuff (meta example is also not yet ported to new api I realised). Ignore my comment then

karlnapf

Thanks for the update.
I'd like to encourage you (again) to focus on c++ code though. These graphical example are not super important. But since you have made the effort in polishing them, let's finish the job (on those you touched), but please don't add new ones to the list for now.
I made a few comments, mostly regarding factories. Try to use them where you can. If none exist, open an issue for it (you don't have to do it here)

Finally, I think we should add those examples to the CI (with a fake matplotlib), so that they get at least executed. Thoughts @vigsterkr ?

karlnapf · 2020-04-04T10:19:54Z

examples/undocumented/python/graphical/converter_ffsep_bss.py

@@ -7,12 +7,9 @@
 Kevin Hughes 2013


question: could we group all the ica examples into one? The code is more or less the same. We could just have a loop over all the different ica methods

karlnapf · 2020-04-04T10:21:10Z

examples/undocumented/python/graphical/util.py

+        return data
+    else:
+        if type == 'binary':
+            return sg.BinaryLabels(data)


labels factory pls

and then you shouldn't need the if then else, as the factory does that

karlnapf · 2020-04-04T10:22:23Z

src/shogun/converter/StochasticProximityEmbedding.cpp

@@ -29,19 +28,19 @@ void StochasticProximityEmbedding::init()
 {
 	SG_ADD(&m_k, "m_k", "Number of neighbors");
 	SG_ADD(&m_tolerance, "m_tolerance", "Regularization parameter");
-	SG_ADD(&m_max_iteration, "max_iteration", "maximum number of iterations");
+	SG_ADD(&m_num_updates, "m_num_updates", "SPE number of updates");
+	SG_ADD(&m_max_iteration, "max_iteration", "Maximum number of iterations");


@gf712 are we still using SG_ADD; or is the templated watch already there?

The PR is mostly ready I think. I can have a look tomorrow!

karlnapf · 2020-04-04T10:24:16Z

src/shogun/converter/StochasticProximityEmbedding.h


-		/** number of apdates per SPE iteration */


"apdates" 😆

karlnapf

looks good, few more minor fixes and then we can merge it

…ssion_sinc.py (cherry picked from commit 876f24c)

(cherry picked from commit d3e5b91)

vinnik-dmitry07 · 2020-04-04T15:52:42Z

Sorry, I remember that I do not have to touch more files but to test nothing breaks after changing this: #4989 (comment) I had to update kernel_ridge_regression.

vinnik-dmitry07 · 2020-04-04T15:54:09Z

Accidentally-closed

(cherry picked from commit ab53d98)

(cherry picked from commit 65e3ca2)

karlnapf · 2020-04-04T17:10:20Z

examples/undocumented/python/graphical/kernel_ridge_regression_sinc.py

+lab = sg.labels(Y.flatten())
+gk = sg.kernel('GaussianKernel', log_width=width)
+gk.init(feat, feat)
+krr = sg.machine('KernelRidgeRegression')


could you use python **kwargs, i.e.
krr = sg.machine('KernelRidgeRregression", labels=lab, kernel=gk, tau=1e-3) (and for all the other instantiations as well). Makes everything look more like Python :)

karlnapf · 2020-04-04T17:10:45Z

examples/undocumented/python/graphical/kernel_ridge_regression_sinc.py

-show()
+plt.plot(XE[0], YE, label='test output')
+plt.plot([XE[0, 200]], [YE200], '+')
+# print(YE[200], YE200)


karlnapf · 2020-04-04T17:11:50Z

src/shogun/converter/StochasticProximityEmbedding.h

 		/** constructor */
 		StochasticProximityEmbedding();

 		/** destructor */
-		virtual ~StochasticProximityEmbedding();
+		~StochasticProximityEmbedding() override;


no virtual destructor?

I follow https://en.cppreference.com/w/cpp/language/virtual#In_detail

More here https://clang.llvm.org/extra/clang-tidy/checks/modernize-use-override.html

Hmm but this is a destructor. You are overriding the base class destructor so there will be leaks..

"override" keyword simply means "this function is marked as virtual in some base class"

https://github.com/chromium/chromium/search?q=override&unscoped_q=override
Chromium guys agree with me.

go for it if you think that is worth it :)

for reference I think @vinnik-dmitry07 means this https://github.com/chromium/chromium/blob/2ca8c5037021c9d2ecc00b787d58a31ed8fc8bcb/tools/clang/plugins/tests/virtual_specifiers.txt

@vinnik-dmitry07 could you hold your horses for now and just follow shogun's style? We don't want to mix up ways we do things, as this leads to redundant discussions and compiler warnings. If you are keen in changing the way this is done in shogun, please open an issue and a separate PR that applies what you want to do to all of shogun.

Of course such a PR is extremely welcome. (Although I suggest you defer this to later and focus on sending something related to your project proposal for now)

karlnapf · 2020-04-04T17:11:58Z

src/shogun/converter/StochasticProximityEmbedding.h


 		/** setter for the maximum number of iterations
 		 *
 		 * @param max_iteration the maximum number of iterations
 		 */
-		void set_max_iteration(const int32_t max_iteration);
+		void set_max_iteration(int32_t max_iteration);


Was the const it causing issues?

Clang-tidy says: https://clang.llvm.org/extra/clang-tidy/checks/readability-avoid-const-params-in-decls.html

karlnapf · 2020-04-04T17:13:43Z

src/shogun/converter/StochasticProximityEmbedding.h

@@ -135,39 +137,36 @@ class StochasticProximityEmbedding : public EmbeddingConverter
 		int32_t get_max_iteration() const;

 		/** get name */
-		virtual const char* get_name() const;
+		const char* get_name() const override;


I am actually not sure what our policy here is, we used to keep the virtual. But it is not needed with override I guess.
@gf712 @vigsterkr thoughts?

This is the correct way to do it, but I think it causes lots of compiler warnings... because we always use virtual... We should write a libtooling script to replace all the virtual get_name with override, or even just use sed

@vinnik-dmitry07 whilst you are at it can you also fix all of these, please?

(cherry picked from commit a2d57fe)

(cherry picked from commit 75dcfc4)

karlnapf

LGTM
thanks a lot for the tidy up!

karlnapf · 2020-04-08T11:35:09Z

somebody should eyeball this and then we can merge it

gf712 · 2020-04-11T10:24:31Z

To be on the safe side can you trigger CI, please? Now all the builds are working again!

vinnik-dmitry07 added 7 commits April 2, 2020 18:19

Fix k-d tree link.

c5afe1f

(cherry picked from commit 49194d0)

Fix formula in KernelDensity description.

35ab782

(cherry picked from commit e7d67d9)

Remove RealFeatures from util.py, mclda.py.

a135005

(cherry picked from commit f8a0a05)

Update matplotlib import in mclda.py.

38e4913

(cherry picked from commit 3da6820)

Minor change in mclda.py: reformat.

aafd635

(cherry picked from commit 56fccb7)

Update matplotlib calls in lda.py.

e237d43

(cherry picked from commit 39fbbb5)

Minor changes to lda.py: reformat.

56a8f97

(cherry picked from commit 1814ebd)

gf712 reviewed Apr 2, 2020

View reviewed changes

karlnapf reviewed Apr 2, 2020

View reviewed changes

vinnik-dmitry07 added 4 commits April 3, 2020 20:46

Change "from x import .." to "import x as .." where x is numpy, shogun.

a5bbb76

(cherry picked from commit 2ad1148)

Correct matplotlib, numpy, shogun import in classifier_gaussian_proce…

e9b52ea

…ss_binary_classification.py. Remove RealFeatures. (cherry picked from commit 597ddd1)

Minor changes to classifier_gaussian_process_binary_classification.py…

0c687e8

…: reformat. (cherry picked from commit 06ec2b3)

Fully fix all commit files.

130f707

(cherry picked from commit 96ef045)

karlnapf reviewed Apr 3, 2020

View reviewed changes

karlnapf reviewed Apr 4, 2020

View reviewed changes

src/shogun/converter/StochasticProximityEmbedding.h

/** number of apdates per SPE iteration */

Copy link

Member

karlnapf Apr 4, 2020

Choose a reason for hiding this comment

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

"apdates" 😆

karlnapf reviewed Apr 4, 2020

View reviewed changes

vinnik-dmitry07 added 2 commits April 4, 2020 18:45

Minor changes + update kernel_ridge_regression.py, kernel_ridge_regre…

e78f24d

…ssion_sinc.py (cherry picked from commit 876f24c)

Compound the examples of converter algorithms.

fb319cb

(cherry picked from commit d3e5b91)

vinnik-dmitry07 closed this Apr 4, 2020

vinnik-dmitry07 reopened this Apr 4, 2020

vinnik-dmitry07 added 2 commits April 4, 2020 19:16

Minor changes.

4b05ad4

(cherry picked from commit ab53d98)

Fix the transfer from BinaryLabels.

38f7d85

(cherry picked from commit 65e3ca2)

vinnik-dmitry07 requested a review from karlnapf April 4, 2020 16:45

vinnik-dmitry07 changed the title ~~util.py, mclda.py, lda.py recovery~~ Update the undocumented graphical python examples Apr 4, 2020

karlnapf reviewed Apr 4, 2020

View reviewed changes

vinnik-dmitry07 added 3 commits April 4, 2020 21:00

Replace put with kwargs.

dc1b03a

(cherry picked from commit a2d57fe)

Correct the formula 2.

66247d7

(cherry picked from commit 75dcfc4)

Revert changing to override and default.

486edda

vinnik-dmitry07 force-pushed the develop branch from 3b7612b to 486edda Compare April 8, 2020 10:22

vinnik-dmitry07 requested a review from karlnapf April 8, 2020 10:23

karlnapf approved these changes Apr 8, 2020

View reviewed changes

Trigger CI.

282fa77

gf712 merged commit 9d625d8 into shogun-toolbox:develop Apr 11, 2020

		@@ -1,32 +1,31 @@
		from pylab import figure,pcolor,scatter,contour,colorbar,show,subplot,plot,connect
		import matplotlib.pyplot as plt

		@@ -1,63 +1,57 @@
		from shogun import RealFeatures
		from shogun import MulticlassLabels

Update the undocumented graphical python examples #4989

Update the undocumented graphical python examples #4989

Conversation

vinnik-dmitry07 commented Apr 2, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karlnapf Apr 2, 2020 • edited

Choose a reason for hiding this comment

vinnik-dmitry07 Apr 2, 2020 • 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

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

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

vinnik-dmitry07 Apr 3, 2020 • edited

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 • 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

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

vinnik-dmitry07 commented Apr 4, 2020

vinnik-dmitry07 commented Apr 4, 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

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 left a comment

Choose a reason for hiding this comment

karlnapf commented Apr 8, 2020

gf712 commented Apr 11, 2020 • edited

karlnapf Apr 2, 2020 •

edited

vinnik-dmitry07 Apr 2, 2020 •

edited

vinnik-dmitry07 Apr 3, 2020 •

edited

karlnapf left a comment •

edited

gf712 commented Apr 11, 2020 •

edited