Feature/factory #4198

karlnapf · 2018-03-05T16:11:32Z

No description provided.

karlnapf · 2018-03-07T15:51:40Z

examples/meta/generator/targets/cpp.json

@@ -11,7 +11,7 @@
    "Comment": "//$comment\n",
    "Init": {
        "Construct": "auto $name = some<C$typeName>($arguments)$kwargs",
-        "Copy": "auto $name = $expr$kwargs",
+        "Copy": "auto $name = wrap($expr)$kwargs",


@lisitsyn re-introduced wrap since we will get memory errors otherwise (things are not ref'ed and then deleted by objects)

This is a bit strange. Copy ctor doesn't ref?

The point here is if you do
'labels = svm->precict'
then Shogun returns a raw pointer. Now if you pass this to another object that increases the ref-counter, and then decreases it, the object is destroyed. So calling a method on it subsequently crashes. This is why I re-added it, just like when you introduced it in the first place.
Or you mean something else?

Yes I've got it now. I guess we change it once we get rid of raw pointers.

karlnapf · 2018-03-07T15:52:23Z

examples/meta/ruby/CMakeLists.txt

@@ -2,6 +2,7 @@ STRING(REGEX REPLACE "(.*)/narray.*$" "\\1" NARRAY_PATH ${NARRAY_LIB})

 LIST(APPEND EXCLUDED_RUBY_META_EXAMPLES
 			base_api-put_get
+      base_api-factory


@vigsterkr same problem as for put: the check for SGMatrix/object bails

(so I blacklisted it for now)

but that should be fixed in develop, so rebase over develop, otherwise i dont understand what's the problem

fixed in develop? I don't remember us fixing that SWIG bug? Did I miss it?
#4177

karlnapf · 2018-03-07T15:53:05Z

examples/meta/src/base_api/factory.sg

+File features_file = csv_file("../../data/classifier_binary_2d_nonlinear_features_train.dat")
+Features features_from_file = features(features_file, enum EPrimitiveType.PT_FLOAT64)
+
+File labels_file = csv_file("../../data/classifier_binary_2d_nonlinear_labels_train.dat")


labels from file will be CDenseLabels which are then converted in the classes (needs some refactor in algorithms, see below)

if you have just added the factory.sg could you change the relative path (../../) to an env var? i'm happy to add the support for that for every language 💃

as if we install every these meta examples, the location of data should be somewhere very else :)

Yep absoltely agree. But I think maybe let's do that for all meta examples in another PR?
I can do that next

karlnapf · 2018-03-07T15:54:11Z

src/shogun/base/class_list.h

 		if (!object)
 		{
-			SG_SERROR("No such class %s", name);
+			SG_SERROR(
+			    "Class %s with primitive type %s does not exist.\n", name,


IMO we need std::iostream for our logs, then we could just overload the << operator and dont have to introduce all these to_string methods ...

although it looks shitty but operator << will not solve your problem to convert some objects to string.. that only works with primitive types

on the other hand having these macros is nice in a long run as we could switch finally our logging backend. maybe spdlog? that allows us to redirect the logs into any sink not only stdout/err

I know but I can overload the << thing, no? Then I dont need the function calls when I write error messages, but can collect them somewhere else?
Totally up for doing a new logging backend. GSoC project part?

new logging backend = just change the macro definitions.
yeah you can offload the operator << but that means that you force to use std::iostream-like interface to be used, which is not the most liked IO back-end (for good reasons) in c++ community

on the other hand you can do the same thing, i.e. implement to_string() for that object where you wanna have operator <<, right?

I think we can introduce an std::cout-like object which supports <<. Internally it could use any logger we want.

karlnapf · 2018-03-07T15:54:43Z

src/shogun/classifier/svm/LibSVM.cpp

@@ -48,7 +48,7 @@ bool CLibSVM::train_machine(CFeatures* data)
 	struct svm_node* x_space;

 	ASSERT(m_labels && m_labels->get_num_labels())
-	ASSERT(m_labels->get_label_type() == LT_BINARY)
+	auto binary_labels = m_labels->as_binary();


@lisitsyn @vigsterkr this is how labels could be converted in algorithms, if they need an interface to CBinaryLabels for example

Looks good.

karlnapf · 2018-03-07T15:55:10Z

src/shogun/features/SparseFeatures.cpp

-template<class ST> CSparseFeatures<ST>::CSparseFeatures(CDenseFeatures<ST>* dense)
+template <class ST>
+CSparseFeatures<ST>::CSparseFeatures(CDenseFeatures<ST>* dense)
+	: CDotFeatures(0), feature_cache(NULL)


this was causing a segfault in python legacy examples every now and then

karlnapf · 2018-03-07T15:55:37Z

src/shogun/labels/BinaryLabels.cpp


 using namespace shogun;

 CBinaryLabels::CBinaryLabels() : CDenseLabels()
 {
 }

+CBinaryLabels::CBinaryLabels(const CBinaryLabels& orig) : CDenseLabels(orig)


I gave labels copy constructors to the sake of converting them, see as_binary below

why do we need a copy? i mean it's over the same datastruct no? do we really want the as_binary be a copy? dont we wanna use c++11's feature of std::move and rvalue?

Yeah I thought about this. The reason why I made it a new instance was the fact that in the as_binary below, all other cases return a new instance, so to be consistent.
And also then the as_binary method can be const. On the other hand, as_binary could return const pointer ...
If you think it is better to just cast, I can do that. Not sure I get what you mean with the move and rvalue

i'm not so sure if i get what this has to do with constness...
i'm talking about the fact that instead of copying the label, what you want is the same memory/object & everything just as a different class, right?

so then use rvalue/move instead of this pattern.

how do you want to deal with converting say multiclass labels (that only contain two classes) to binary? For this, the actual vector needs to be changed, so a new instance is needed.

For converting the same datastruct, I see the point

I think it is fine to copy BinaryLabels most of the time. What should be rather copy-on-write is the data beneath.

I would actually prefer if this thing gave you a read-only labels object (we wanna have immutable labels anyways)
Note that memory is not copied here.
Also, after reading on it, I think the move constructor stuff doesnt really help here

karlnapf · 2018-03-07T15:56:21Z

src/shogun/labels/DenseLabels.cpp

@@ -77,15 +83,15 @@ void CDenseLabels::set_labels(SGVector<float64_t> v)
 	m_labels = v;
 }

-SGVector<float64_t> CDenseLabels::get_labels()
+SGVector<float64_t> CDenseLabels::get_labels() const


I made lots of getters const

karlnapf · 2018-03-07T16:02:12Z

src/shogun/labels/DenseLabels.h

+
+		virtual const char* get_name() const override
+		{
+			return "DenseLabels";


@lisitsyn made the class non-abstract, for the sake of loading from files (one doenst know what type of labels it is at that point, only when the algorithm tries to convert to say binary labels we know)

karlnapf · 2018-03-07T16:02:39Z

src/shogun/labels/Labels.cpp

 {
 	return m_current_values;
 }
+
+Some<CBinaryLabels> CLabels::as_binary() const


mentioned conversion method to convert label types into each other

see above regarding move/rvalue

karlnapf · 2018-03-07T16:03:21Z

src/shogun/lib/SGVector.cpp

@@ -927,7 +927,7 @@ void SGVector<T>::scale(T alpha)

 template<class T> void SGVector<T>::load(CFile* loader)
 {
-	REQUIRE(loader, "Require a valid 'c FILE pointer'\n");


great error message for users

karlnapf · 2018-03-07T16:03:50Z

src/shogun/statistical_testing/QuadraticTimeMMD.cpp

@@ -66,7 +67,7 @@ struct CQuadraticTimeMMD::Self
 	SGVector<float64_t> gamma_fit_null();

 	CQuadraticTimeMMD& owner;
-	unique_ptr<CMultiKernelQuadraticTimeMMD> multi_kernel;
+	shared_ptr<CMultiKernelQuadraticTimeMMD> multi_kernel;


meta examples now use some so cannot have a unique ptr inside shogun and expose the raw pointer via SWIG

but this way you loose:

std::move the thing around - maybe not so important

enforce sole ownership of the resource.... @lambday was this intentional to have it as unique? i would presume yes....

discussed with him, he was fine with this change. Alternative would be to always return a new instance in the ::multiclass() method (which currently returns the raw pointer)

karlnapf · 2018-03-07T16:05:13Z

src/shogun/util/factory.h

+		return result;
+	}
+
+	CLabels* labels(CFile* file)


@vigsterkr @lisitsyn this is the stuff behind the factory API

I like the approach. It would become the main interface.

vigsterkr · 2018-03-07T16:24:32Z

If u need to have new mem of course u need a new instance but thats not always the case. Hence have both of them and the compiler will figure it out for u

…

On 7 Mar 2018, at 17:22, Heiko Strathmann ***@***.***> wrote: @karlnapf commented on this pull request. In src/shogun/labels/BinaryLabels.cpp: > using namespace shogun; CBinaryLabels::CBinaryLabels() : CDenseLabels() { } +CBinaryLabels::CBinaryLabels(const CBinaryLabels& orig) : CDenseLabels(orig) For converting the same datastruct, I see the point — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

vigsterkr · 2018-03-07T16:57:19Z

Btw now thinking of this i’m not so sure we need this things explicit... as it makes the whiole thing less smooth.... afaik you could have the whole thing implicit with the right ctors

…

On 7 Mar 2018, at 17:22, Heiko Strathmann ***@***.***> wrote: @karlnapf commented on this pull request. In src/shogun/labels/BinaryLabels.cpp: > using namespace shogun; CBinaryLabels::CBinaryLabels() : CDenseLabels() { } +CBinaryLabels::CBinaryLabels(const CBinaryLabels& orig) : CDenseLabels(orig) For converting the same datastruct, I see the point — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

lisitsyn · 2018-03-07T18:43:47Z

src/shogun/classifier/svm/LibSVM.cpp

@@ -48,7 +48,7 @@ bool CLibSVM::train_machine(CFeatures* data)
 	struct svm_node* x_space;

 	ASSERT(m_labels && m_labels->get_num_labels())
-	ASSERT(m_labels->get_label_type() == LT_BINARY)
+	auto binary_labels = m_labels->as_binary();


Looks good.

lisitsyn · 2018-03-07T18:45:27Z

src/shogun/labels/BinaryLabels.cpp


 using namespace shogun;

 CBinaryLabels::CBinaryLabels() : CDenseLabels()
 {
 }

+CBinaryLabels::CBinaryLabels(const CBinaryLabels& orig) : CDenseLabels(orig)


I think it is fine to copy BinaryLabels most of the time. What should be rather copy-on-write is the data beneath.

lisitsyn · 2018-03-07T18:47:13Z

src/shogun/lib/DataType.h

+ * @param pt primitive type
+ * @return string for primitive type
+ */
+std::string ptype(EPrimitiveType pt);


I like primitive_name a bit better.

will update

lisitsyn · 2018-03-07T18:48:32Z

src/shogun/util/factory.h

+		return result;
+	}
+
+	CLabels* labels(CFile* file)


I like the approach. It would become the main interface.

karlnapf · 2018-03-12T15:31:27Z

We could also have a constructor in CBinaryLabels that takes a CLabels pointer and then does the same thing as this introduced as_binary method. Not sure that conflicts with a copy constructor in some way. I actually like the explicitness

karlnapf · 2018-03-13T10:28:42Z

@lisitsyn see updated version

vigsterkr · 2018-03-14T14:19:51Z

src/shogun/base/SGObject.h

+	 * @return The requested type
+	 */
+	template <class T>
+	const T* as() const


i wouldn't copy-paste this... some parts of it at least can be merged

vigsterkr · 2018-03-14T14:21:40Z

src/shogun/base/some.h

@@ -153,6 +153,11 @@ namespace shogun
 		return value;
 	}

+	inline const char* wrap(const char* ptr)


for inline-ness plz use the macro: SG_FORCED_INLINE
which will make it sure that it's really inlined

just replace it with the given macro above.

in case you insist on using inline... otherwise just drop the keyword inline

I don't insist, I am just following the rest of the file, also it doesnt have any shogun includes

but let me remove the keyword

dont get it why is it better to remove instead of using a macro that ensures the inline-ess... hence make the whole code faster... ¯_(ツ)_/¯

vigsterkr · 2018-03-14T14:23:40Z

src/shogun/labels/BinaryLabels.cpp

+
+namespace shogun
+{
+	Some<CBinaryLabels> binary_from_binary(CBinaryLabels* orig)


these are helper methods, could also move the contents directly into the switch below

cpp only, nevermind

vigsterkr · 2018-03-14T14:26:19Z

src/shogun/labels/BinaryLabels.cpp

+				    labels[i])
+			}
+		}
+		auto result = new CBinaryLabels(labels);


afaik this copies the whole vvector itself.... do we really need to copy the data where it doesn't change at all....

it copies indeed, which I actually didnt realise. Why does it copy? Doesnt seem to make sense to me. And agree, it should not

vigsterkr · 2018-03-14T14:27:41Z

src/shogun/labels/BinaryLabels.cpp

+		return Some<CBinaryLabels>::from_raw(orig);
+	}
+
+	Some<CBinaryLabels> binary_from_dense(CDenseLabels* orig)


que?
do we need to spell this one out? does this need to be expose to anybody apart from ourself internally?

ah this is in .cpp. nevermind

no this is only internally, didnt put it in the header.
You think it should be just inside the switch?

see the 2nd comment... it's foobar, just ignore the first comment

browser didnt reload ;)

vigsterkr · 2018-03-14T14:31:07Z

src/shogun/labels/MulticlassLabels.cpp

+				(*std::min_element(unique.begin(), unique.end())) == 0 &&
+				(*std::max_element(unique.begin(), unique.end())) == (index_t)unique.size()-1,
+				"Multiclass labels must be contiguous integers in [0, ..., num_classes -1].\n"


can we fixme/todo this... as this should not be a hard requirement, right?

i mean one would like to have something even like a: ['foo', 'bar', 'zelda'] multiclass labeling possible sooner than later

vigsterkr · 2018-03-14T14:35:58Z

tests/meta/tester.cpp

@@ -62,6 +83,6 @@ int main(int argc, const char *argv[])
 	SG_UNREF(a_ref);

 	exit_shogun();
-    return equal != true;
+	return (loaded_equals_ref && ref_equals_loaded) != true;


we could drop the != true part and just have a negation, right?

vigsterkr · 2018-03-14T14:37:03Z

tests/unit/utils/factory_unittest.cc

+	ASSERT_TRUE(cast != nullptr);
+	auto loaded_mat = cast->get_feature_matrix();
+	EXPECT_TRUE(loaded_mat.equals(mat));
+	delete obj;


delete the file as well plz.

make some methods const, add copy constructors, add CBinaryLabels::from(...)

Populate for kernel, machine, features, labels

Includes LibSVM using new CLabels::as_binary()

due to typemap bug, shogun-toolbox#4177

-don't copy memory when converting from dense to binary labels -delete temporary files in unit tests -clean ups -remove unused const version of CSGObject::as

-add == operator for Some -clean-ups

karlnapf · 2018-03-20T17:34:22Z

Alright, let's see what the buildbot says to this...

karlnapf force-pushed the feature/factory branch 3 times, most recently from e8ca7d1 to 822504d Compare March 7, 2018 13:08

karlnapf commented Mar 7, 2018

View reviewed changes

lisitsyn approved these changes Mar 7, 2018

View reviewed changes

karlnapf force-pushed the feature/factory branch from a636b4f to faf7f6e Compare March 14, 2018 10:12

vigsterkr reviewed Mar 14, 2018

View reviewed changes

karlnapf added 27 commits March 19, 2018 15:56

add arugment for primitive type to create_object

ae8a7b9

clean up error message

5c7c5de

add CLabels::as_binary converter

38e7e2f

make some methods const, add copy constructors, add CBinaryLabels::from(...)

Introduce new factory for creating objects

f2dcf6c

Populate for kernel, machine, features, labels

rename objects to factory and add examples for labels and features

e789605

Port kernel svm example to using features/labels factory

c85fedd

Includes LibSVM using new CLabels::as_binary()

fix meta example segfault caused by mixing some and unique_ptr

77f4c83

exclude factory API example

567513e

due to typemap bug, shogun-toolbox#4177

fix potential segfault due to forgotten member initialization

30c0502

apply autoformatter

ef90e8a

refactor binary_labels conversion and don't use copy ctor anymore

e472328

rename ptype to ptype_name

607453a

minor error message update

e582c35

add missing import

457029d

add multiclass_labels converter and use in meta example

eea2b05

add regression_labels converter and use in meta example

8a3c8e6

fix segfault via add forgotten ref-counter

a48afd6

add newlines to error messages

23d035c

fix more segfaults due to missing SG_REF

44c0859

address PR comments

06e2668

-don't copy memory when converting from dense to binary labels -delete temporary files in unit tests -clean ups -remove unused const version of CSGObject::as

add == operator and fix downstream segfault

ab082a1

also take over scores in regression_from_dense

066d0e0

add unit tests for label conversion

ed8d211

-add == operator for Some -clean-ups

fix more segfaults due to missing SG_REF

4520e32

close file before closing to avoid errors on windows

c827927

apply autoformater

ef97350

replace inline with SG_FORCED_INLINE

7b43b50

karlnapf force-pushed the feature/factory branch from 9be4cfe to 7b43b50 Compare March 19, 2018 16:00

karlnapf merged commit 2362d20 into shogun-toolbox:develop Mar 20, 2018

Feature/factory #4198

Feature/factory #4198

Conversation

karlnapf commented Mar 5, 2018

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

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

vigsterkr commented Mar 7, 2018 via email

vigsterkr commented Mar 7, 2018 via email

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 commented Mar 12, 2018

karlnapf commented Mar 13, 2018

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