New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
No more dynamic dispatching in CSGObject::put #4276
No more dynamic dispatching in CSGObject::put #4276
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gr8, i've added some initial comments/suggestions.
src/shogun/base/SGObject.h
Outdated
@@ -39,6 +39,30 @@ class Parameter; | |||
class CSerializableFile; | |||
class ParameterObserverInterface; | |||
|
|||
// all shogun base classes for put/add templates |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great but i recon since here is everything a fwd declaration as SGObject.h is already quite cramped with stuff maybe it'd be better to factor this out somewhere else and just include that header in this header
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good call, will move them!
void put(const std::string& name, CSGObject* value); | ||
template <class T, | ||
class X = typename std::enable_if<is_sg_base<T>::value>::type, | ||
class Z = void> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why the Z
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
because SWIG confuses it with the double templated put (for non-objects) and fails.
%template(put) CSGObject::put<CMachine, CMachine, void>;
src/shogun/base/SGObject.h
Outdated
class CMulticlassStrategy; | ||
|
||
template <class T> | ||
struct is_sg_base |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i wonder whether actually we could generate from a constexpr or something as for a while i imagine that this template will be changed....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this will be changed definitely.
I dont know exactly what you have in mind there. Have an example?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You might specialize is_sg_base for specific T
.
a42cdca
to
06c07f3
Compare
(no more dynamic dispatching)
06c07f3
to
c0fce21
Compare
@@ -239,4 +239,18 @@ namespace shogun | |||
%template(get_int_vector) CSGObject::get_vector_as_matrix_dispatcher<SGMatrix<int32_t>, int32_t>; | |||
#endif // SWIGJAVA | |||
|
|||
%define PUT_ADD(sg_class) | |||
%template(put) CSGObject::put<sg_class, sg_class, void>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
need to have this as put/add are now templates.
Also explains why I need 3 template parameters for put (there already is one with 2)
return; | ||
} | ||
|
||
Tag<std::vector<CKernel*>> tag_array_kernel(name); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this doesnt happen anymore now as we have the type information
@@ -1121,3 +1049,9 @@ CSGObject* CSGObject::get(const std::string& name) | |||
self->map[BaseTag(name)].get_value().type().c_str()); | |||
return nullptr; | |||
} | |||
|
|||
void CSGObject::push_back(CDynamicObjectArray* array, CSGObject* value) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
need this as cannot include the dynamic object array header in the SGObject.h
…hogun-toolbox#4276) (no more dynamic dispatching)
No description provided.