Skip to content

Commit

Permalink
fix put and getType for interface
Browse files Browse the repository at this point in the history
  • Loading branch information
@vigsterkr
vigsterkr committed Feb 7, 2018
1 parent df63984 commit 971d67d
Show file tree
Hide file tree
Showing 3 changed files with 22 additions and 59 deletions.
10 changes: 10 additions & 0 deletions src/interfaces/swig/shogun.i
View file
Expand Up @@ -116,6 +116,16 @@

%include "ParameterObserver.i"

%define SUPPORT_TAG(short_type, type)
%template(put) shogun::CSGObject::put<type, void>;
%template(get_ ## short_type) shogun::CSGObject::get<type, void>;
%enddef
SUPPORT_TAG(double, float64_t)
SUPPORT_TAG(int, int64_t)
SUPPORT_TAG(real_vector, SGVector<float64_t>)
SUPPORT_TAG(real_matrix, SGMatrix<float64_t>)
SUPPORT_TAG(object, CSGObject*)

#if defined(SWIGPERL)
%include "abstract_types_extension.i"
#endif
44 changes: 0 additions & 44 deletions src/shogun/base/SGObject.cpp
View file
Expand Up @@ -1012,47 +1012,3 @@ CSGObject* CSGObject::create_empty() const
SG_REF(object);
return object;
}

namespace shogun
{
#define SGOBJECT_PUT_DEFINE(T) \
void CSGObject::put(const std::string& name, T const& value) throw( \
ShogunException) \
{ \
Tag<T> tag(name); \
put(tag, value); \
}

SGOBJECT_PUT_DEFINE(SGVector<int32_t>)
SGOBJECT_PUT_DEFINE(SGVector<float64_t>)
SGOBJECT_PUT_DEFINE(CSGObject*)

#define PUT_DEFINE_CHECK_AND_CAST(T) \
else if (has(Tag<T>(name))) put(Tag<T>(name), (T)value);

/* Some target languages have problems with scalar numeric types, so allow to
* convert all int/float types into each other.
*
* For example, Octave treats a=1.0 as an integer, and b=1.1 as a float.
* Furthermore, if a user wants to set a registered 16bit integer using a
* literal obj.put("16-bit-var", 2), might complain about a wrong type since
* internally the int literal is represented at a different word length. */
#define SGOBJECT_PUT_DEFINE_WITH_CONVERSION(numeric_t) \
void CSGObject::put( \
const std::string& name, \
numeric_t const& value) throw(ShogunException) \
{ \
/* use correct type of possible, otherwise cast-convert */ \
if (has(Tag<numeric_t>(name))) \
put(Tag<numeric_t>(name), value); \
PUT_DEFINE_CHECK_AND_CAST(int32_t) \
PUT_DEFINE_CHECK_AND_CAST(float32_t) \
PUT_DEFINE_CHECK_AND_CAST(float64_t) \
else /* if nothing works, moan about original type */ \
put(Tag<numeric_t>(name), value); \
}

SGOBJECT_PUT_DEFINE_WITH_CONVERSION(int32_t)
SGOBJECT_PUT_DEFINE_WITH_CONVERSION(float32_t)
SGOBJECT_PUT_DEFINE_WITH_CONVERSION(float64_t)
};
27 changes: 12 additions & 15 deletions src/shogun/base/SGObject.h
View file
Expand Up @@ -364,21 +364,18 @@ class CSGObject
}
}

#define SGOBJECT_PUT_DECLARE(T) \
/** Setter for a class parameter, identified by a name. \
* Throws an exception if the class does not have such a parameter. \
* \
* @param name name of the parameter \
* @param value value of the parameter along with type information \
*/ \
void put(const std::string& name, T const& value) throw(ShogunException);

SGOBJECT_PUT_DECLARE(int32_t)
SGOBJECT_PUT_DECLARE(float32_t)
SGOBJECT_PUT_DECLARE(float64_t)
SGOBJECT_PUT_DECLARE(SGVector<int32_t>)
SGOBJECT_PUT_DECLARE(SGVector<float64_t>)
SGOBJECT_PUT_DECLARE(CSGObject*)
/** Setter for a class parameter, identified by a name.
* Throws an exception if the class does not have such a parameter.
*
* @param name name of the parameter
* @param value value of the parameter along with type information
*/
template<class T>
void put(const std::string& name, const T value) throw(ShogunException)
{
Tag<T> tag(name);
put(tag, value);
}

/** Getter for a class parameter, identified by a Tag.
* Throws an exception if the class does not have such a parameter.
Expand Down

3 comments on commit 971d67d

@karlnapf
Copy link
Member

@karlnapf karlnapf commented on 971d67d Feb 7, 2018 •
edited

Choose a reason for hiding this comment

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

Two problems here, which is why I was trying the other things (apart from that it doesnt compile)

  1. When using this from the cpp interface, one gets "wrong" type when doing
    svm.put("kernel", kernel("Gaussian"))
    This is since "kernel" is registered as CSGObject but since the put here is templated, it is matched against CKernel*. So @lisitsyn and I decided to make it not templated but explicit only for CSGObject*
    To reproduce, run the meta examples for cpp (kwargs.sg)

  2. The Octave "float as int" problem. One could argue that this can be solved with %extend. However, there is the general problem that 4.0 might have a different type in the target languages (is it int32 everywhere?). In particular for scalar literals (to set parameters), it might be annoying from an interface perspective if we enforce the word length rather than just accepting anything (maybe warning) and converting.
    To reproduce, again run kwargs.

@vigsterkr
Copy link
Member Author

Choose a reason for hiding this comment

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

:)

  1. i considered fixed in 2f488e4
    test both the cpp kwargs (you get a nice deprecation warning) and as well in python interface --- see the travis CI for this branch was failing because:

    1. cpp kwargs problem that is fixed now with the above mentioned patch
    2. in java DoubleMatrix is not imported got SGObject java wrapper --- looking into
    3. that is actually your 2)

  2. needs similar hacks in the swig interface definition as dynobjarray.

@vigsterkr
Copy link
Member Author

@vigsterkr vigsterkr commented on 971d67d Feb 7, 2018 •
edited

Choose a reason for hiding this comment

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

btw: RE 1) @lisitsyn @karlnapf
my deprecation message actually summarizes what i think of this issue. if type-checking is actually done over a registered class then i dont understand why dont we narrow it down to the reasonable level. namely of course it will scream the has<T> that the supplied object is not in the right type as you expect CSGObject for a kernel. that parameter shoudl be registered as CKernel as its declared type in the class not as CSGObject... that way actually we would do some good for us as now you can actually put a KernelMachine or Distance or whatever SGObject derived stuff to that particular parameter kernel, nothing will scream at you when you add if you pass it as CSGObject*.... imo that's actually a bug in the param registration end and just shooting ourselves in the leg, i.e. prone to error later... of course something somewhere will catch it when you actually start training the kernel machine. but wouldn't it be nicer to already catch it at put?

actually i think that the hacky-fix of 2f488e4 should move into has and not in put. we should just do the upcasting there but for really a limited time until param registration for base classes are supported; like CKernel, CFeatures etc.

Please sign in to comment.