Auto dispatch create methods in python #5118

ghost · 2020-09-02T03:25:51Z

This fixes #4998 using the same technique that @gf712 suggested. I created a visitor for interface classes and changed the create method in class_list to accept the visitor. Then used this visitor in swig to transform the interface object to PyObject.

vigsterkr · 2020-09-02T07:15:22Z

src/shogun/base/base_types.h

+		MachineEvaluation, StructuredModel, FactorType, ParameterObserver,
+		Distribution, GaussianProcess, Alphabet>;
+
+	namespace types_detail


gf712 · 2020-09-02T08:40:21Z

src/interfaces/python/factory_python.i

+	class ShogunInterfaceToPyObject : public InterfaceTypeVisitor
+	{
+	public:
+		virtual void on(std::shared_ptr<SGObject>* v)


minor thing, but now we are trying to always use override instead of overwriting virtual functions

Suggested change

virtual void on(std::shared_ptr<SGObject>* v)

void on(std::shared_ptr<SGObject>* v) override

override FTW

gf712 · 2020-09-02T08:41:15Z

src/interfaces/python/factory_python.i

+// to query the swig_type in run time and be less implementation dependent maybe?
+// but this is faster and should not cause problems
+#define SHOGUN_GET_SWIG_TYPE(name)                                             \
+	SWIGTYPE_p_std__shared_ptrT_shogun__##name##_t


I am wondering if this can be generalised to the other swig interfaces?

I'm assuming you mean the whole file. I would guess so. factory_python is a horrible name for that regard. Any name suggestions?
Edit: I will call it factory_visitors.i for now

yes, that's fine. And yes, I meant if you could for example template ShogunInterfaceToPyObject, and the template determines the interface object type, e.g. ShogunInterfaceFactory<PyObject>.

I have two reasons not to:

Added complexity without any other use case at the moment.

After reading "GetterVisitor" I don't see any place for this visitor in the future (and maybe not for InterfaceClassVisitor either). Once the parameter framework and AnyVisitor support base types we can get rid of both new visitors, and use GetterVisitor directly, no?

Since R is working now, I should make this generic as you suggested :"D

I think so, that would be great! Then it could be easier to add other languages?

Yeah. This should work with octave as well.

gf712 · 2020-09-02T08:43:40Z

src/interfaces/python/factory_python.i

+					nullptr, nullptr, SHOGUN_GET_SWIG_TYPE(SGObject),
+					SWIG_POINTER_OWN);
+			}
+			return m_pyobj;


even if it should not be possible to reach this, could you replace this with an exception? The worst thing that can happen is segfault in Python, because then the interpreter crashes. An exception can at least be handled gracefully

In this case I return a python null object. I will change the function call to SWIG_Py_Void() to be more clear and add a comment.

Scratch that. Throwing is better.

yes, agreed, throwing would provide more information. Getting None from a factory might be odd for a user

ghost · 2020-09-02T15:43:03Z

I can see this visitor being integrated into the GetterVisitor (making AnyVisitor inherit from InterfaceTypeVisitor for example, and defaulting the new methods to on(SGObject)).
Does the parameter framework currently support "getting" objects of base classes?
If no, I think this structure should be good for now.

gf712 · 2020-09-02T16:32:12Z

Does the parameter framework currently support "getting" objects of base classes?

Not yet, it's been worked on in #4793

gf712 · 2020-12-08T17:24:42Z

src/shogun/util/visitors/InterfaceTypeVisitor.h

+		virtual void on(std::shared_ptr<ParameterObserver>*) = 0;
+		virtual void on(std::shared_ptr<Distribution>*) = 0;
+		virtual void on(std::shared_ptr<GaussianProcess>*) = 0;
+		virtual void on(std::shared_ptr<Alphabet>*) = 0;


I wonder if there could be a way to automatically generate this? Then again the base types probably do not change that often

We can do type erasure as follows

virtual void on(std::shared_ptr<void>*) = 0;

and then try to dynamic_cast for all base types one by one (with generic for loop on types) and dispatch to a generic on.

template <typename BaseClass> void on(std::shared_ptr<BaseClass>*);

But this is slower.

We can alaso manually construct the virtual table of on methods using a polymorphic lambda. So we would have something like

// vtable is constructed from one polymorphic lambda std::tuple<std::function<void(std::shared_ptr<BaseClasses>*)>...> vtable; template <typename BaseClass> void on(std::shared_ptr<BaseClass>* ptr) { std::get<TypeToIndex<BaseClass>::value>(this->vtable) (ptr); }

This should be just as efficient as multiple virtual methods, but it requires complex code.

I think the manual vtable is similar to what we have in Any and use in SGObject

shogun/src/shogun/base/SGObject.h

Line 768 in 9b8d856

void register_parameter_visitor() const

I wonder if the same logic could be used. I guess it would only be possible if we add another base class between SGObject and the "base" classes, where it would register types to be used (somehow) in the interfaces

The type erasure solution is kind of what we have/had in SGObject but we are moving away from that and just use visitor patterns.

stale · 2021-06-07T20:35:31Z

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-06-14T21:27:15Z

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

theartful added 4 commits September 2, 2020 05:13

Auto dispatch create methods in python

a616d71

bug fixes

87cba0b

Rearrange types from most derived to least

f54957b

Throw if class does not exist

3139635

vigsterkr reviewed Sep 2, 2020

View reviewed changes

gf712 reviewed Sep 2, 2020

View reviewed changes

Address comments and fix bugs

a3fefc4

Update notebooks

e73f55f

gf712 reviewed Dec 8, 2020

View reviewed changes

stale bot added the stale label Jun 7, 2021

stale bot closed this Jun 14, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Auto dispatch create methods in python #5118

Auto dispatch create methods in python #5118

ghost commented Sep 2, 2020

vigsterkr Sep 2, 2020

gf712 Sep 2, 2020

ghost Sep 2, 2020

gf712 Sep 2, 2020

ghost Sep 2, 2020 •

edited by ghost

gf712 Sep 2, 2020

ghost Sep 2, 2020

ghost Dec 8, 2020

gf712 Dec 8, 2020

ghost Dec 8, 2020

gf712 Sep 2, 2020 •

edited

ghost Sep 2, 2020

ghost Sep 2, 2020

gf712 Sep 2, 2020

ghost commented Sep 2, 2020 •

edited by ghost

gf712 commented Sep 2, 2020

gf712 Dec 8, 2020

ghost Dec 8, 2020 •

edited by ghost

ghost Dec 8, 2020 •

edited by ghost

gf712 Dec 9, 2020

gf712 Dec 9, 2020

stale bot commented Jun 7, 2021

stale bot commented Jun 14, 2021

	virtual void on(std::shared_ptr<SGObject>* v)
	void on(std::shared_ptr<SGObject>* v) override

Auto dispatch create methods in python #5118

Auto dispatch create methods in python #5118

Conversation

ghost commented Sep 2, 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

ghost Sep 2, 2020 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gf712 Sep 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

ghost commented Sep 2, 2020 • edited by ghost

gf712 commented Sep 2, 2020

Choose a reason for hiding this comment

ghost Dec 8, 2020 • edited by ghost

Choose a reason for hiding this comment

ghost Dec 8, 2020 • edited by ghost

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stale bot commented Jun 7, 2021

stale bot commented Jun 14, 2021

ghost Sep 2, 2020 •

edited by ghost

gf712 Sep 2, 2020 •

edited

ghost commented Sep 2, 2020 •

edited by ghost

ghost Dec 8, 2020 •

edited by ghost

ghost Dec 8, 2020 •

edited by ghost