Use visitors in put and get #5037

gf712 · 2020-05-18T15:47:07Z

This replaces the make_any in put with a visitor pattern (saves creating a new Any and replacing the old with the new one; instead we change the stored value directly with the visitor). For symmetry I also do the same with get, but I don't think it makes any difference to using any_cast. Also had to change some of the getter signatures to have access to mutable and immutable references.

This will be really useful to register things like std::optional and std::variant without changing the functionality in Any.

gf712 · 2020-05-18T15:48:17Z

src/shogun/base/SGObject.h

@@ -298,30 +310,20 @@ class SGObject: public std::enable_shared_from_this<SGObject>
 	 */
 	template <typename T,
 		      typename std::enable_if_t<!is_string<T>::value>* = nullptr>
-	void put(const Tag<T>& _tag, const T& value) noexcept(false)


removed these as they don't do anything for us. This is just useful if the template type determines if it is noexcept

gf712 · 2020-05-18T15:48:51Z

src/shogun/base/SGObject.h

-	{
-		const Any value = get_parameter(_tag).get_value();
-		try
+		if constexpr (is_string<T>::value)


simplified a bunch of overloads with if constexpr

gf712 · 2020-05-18T15:49:37Z

src/shogun/base/SGObject.h

+				}
+			);
+		}
+	}


@vigsterkr I adapted from your PR. Can then put all the visitor registrations in this function

gf712 · 2020-05-18T15:50:20Z

src/shogun/base/SGObject.h

@@ -1065,15 +1087,70 @@ class SGObject: public std::enable_shared_from_this<SGObject>
 	 * @param _tag name information of parameter
 	 * @param value new value of parameter
 	 */
-	void update_parameter(const BaseTag& _tag, const Any& value, bool do_checks = true);
+	template <typename T>
+	void update_parameter(const BaseTag& _tag, const T& value, bool do_checks = true)


made this template dependent so that we can have all these parameter handling branches in a single location

gf712 · 2020-05-18T15:50:55Z

src/shogun/base/SGObject.h

+		}
+		if constexpr (std::is_same_v<T, Any>)
+		{
+			param.set_value(value);


this is needed right now for auto init, but I will refactor it in another PR and can then remove this

gf712 · 2020-05-18T15:51:24Z

src/shogun/lib/any.h

@@ -1229,7 +1225,7 @@ namespace shogun
 		static void register_caster(std::function<To(From)> caster);

 		template <class Type, class State>
-		static void register_visitor(std::function<void(Type, State*)> visitor);
+		static void register_visitor(std::function<void(Type*, State*)> visitor);


it is more convenient to pass around pointers rather than values

gf712 · 2020-05-19T06:20:13Z

src/shogun/base/SGObject.h

@@ -1160,6 +1237,7 @@ class SGObject: public std::enable_shared_from_this<SGObject>
 	{
 		auto param = this->get_parameter(BaseTag(name));
 		auto cloned = any_cast<T>(param.get_value());
+		param.get_properties().remove_property(ParameterProperties::FUNCTIONAL);


This is a bit awkward, but the observers pass on properties as is, meaning the value of a function call still has a FUNCTIONAL property, eg cluster centres in KMeans unit test

gf712 · 2020-05-19T07:32:46Z

src/shogun/base/SGObject.h

+			ParameterProperties::READONLY | ParameterProperties::FUNCTIONAL);
+		std::function<T()> bind_method = [this, method](){
+			return (static_cast<const S*>(this) ->* method)();
+		};


this should be more efficient than std::bind

karlnapf · 2020-05-19T09:10:42Z

src/shogun/base/AnyParameter.h

 		READONLY = 1u << 11,

-		// an executable function
-		RUNFUNCTION = 1u << 12,
+		// a class member function with side effects


do we want to allow this even? I'd prefer if we didnt ... landslide risk

This is used by some classes, like RandomMixin to set the random seed (set_random_seed is not const). The safety net we have here is that you can only call these functions using run, rather than get.

vigsterkr

⛵

vigsterkr · 2020-05-22T05:48:24Z

.ci/ci.yml

@@ -62,6 +62,7 @@ jobs:
 - job: swig
  displayName: Linux SWIG Interface
  dependsOn: libshogun
+  timeoutInMinutes: 90


as well? :D

it should be unrelated. It's just with the python notebook job it can go just above 60 minutes, so just set it to 90

vigsterkr · 2020-05-22T05:51:30Z

src/shogun/base/SGObject.h

+				}
+			);
+		}
+	}


gf712 requested review from vigsterkr, lisitsyn and karlnapf May 18, 2020 15:47

gf712 commented May 18, 2020

View reviewed changes

gf712 force-pushed the sg_put_get_visitor branch from 16722af to 5788c64 Compare May 18, 2020 16:49

put get visitor

8cd9337

gf712 force-pushed the sg_put_get_visitor branch from 5788c64 to 8cd9337 Compare May 18, 2020 21:26

gf712 commented May 19, 2020

View reviewed changes

replace std::bind

dd7bc9d

gf712 force-pushed the sg_put_get_visitor branch from d34de12 to dd7bc9d Compare May 19, 2020 07:13

gf712 commented May 19, 2020

View reviewed changes

karlnapf reviewed May 19, 2020

View reviewed changes

clean up parameter properties

c112da0

gf712 force-pushed the sg_put_get_visitor branch from f8ece9a to c112da0 Compare May 19, 2020 09:56

gf712 mentioned this pull request May 19, 2020

Extend observe() to emit std::function objects #5033

Closed

gf712 force-pushed the sg_put_get_visitor branch 2 times, most recently from 03b211c to 6b643aa Compare May 19, 2020 18:00

cleaned up repetitive checks for parameter presence

cfa5305

gf712 force-pushed the sg_put_get_visitor branch from 6b643aa to cfa5305 Compare May 19, 2020 18:02

vigsterkr approved these changes May 22, 2020

View reviewed changes

gf712 merged commit 93a649b into shogun-toolbox:develop May 22, 2020

gf712 deleted the sg_put_get_visitor branch June 2, 2020 06:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use visitors in put and get #5037

Use visitors in put and get #5037

gf712 commented May 18, 2020 •

edited

gf712 May 18, 2020

gf712 May 18, 2020

gf712 May 18, 2020

vigsterkr May 22, 2020

gf712 May 18, 2020

gf712 May 18, 2020

gf712 May 18, 2020

gf712 May 19, 2020 •

edited

gf712 May 19, 2020

karlnapf May 19, 2020

gf712 May 19, 2020

vigsterkr left a comment

vigsterkr May 22, 2020

gf712 May 22, 2020

vigsterkr May 22, 2020

Use visitors in put and get #5037

Use visitors in put and get #5037

Conversation

gf712 commented May 18, 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

gf712 May 19, 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

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

gf712 commented May 18, 2020 •

edited

gf712 May 19, 2020 •

edited