Add meta example features-char-string #4423

avramidis · 2018-11-23T16:22:59Z

A simple meta example for CStringFeatures.

I would like to make changes and add output file for an integration test but I am not sure if the current outputs are enough for that. Currently, it stores "max_string_length", "number_of_strings" and "length_of_first_string". I don't think that is possible to practically check all the values of "strings".

However, if you don't have a better idea I could add eight variables that store the value of the first vector before and after the change to "test".

karlnapf

A made a few comments.

Does this compile/execute? What about the other cpp examples, do they still work?
Also, does it work from other targets (at least python/java?). We will have our CI back to check this for you soon ...

examples/meta/generator/targets/cpp.json

src/shogun/features/StringFeatures.h

karlnapf · 2018-11-23T17:18:13Z

You are right we dont really need any output here, as nothing really happens. THese outputs are for making sure that algorithms dont all of a sudden output different results.

karlnapf · 2018-11-26T16:28:07Z

Our CI (before PR) is down atm. Can you confirm this example runs smooth locally (say c++, python, and maybe java)?

avramidis · 2018-11-27T09:50:07Z

Apologies for the late reply.

karlnapf · 2018-11-30T12:27:52Z

Shall we update and merge this one?

avramidis · 2018-11-30T12:30:29Z

Yes, that is the plan. I have started making changes but they will be ready mid next week if that is ok with you.

karlnapf · 2018-11-30T12:34:22Z

Sure of course.

avramidis · 2019-01-14T13:54:49Z

An update for this one - I am working on refactoring the watch_method function to be able to register functions that take arguments. The features-char-string example needs that. I think made some progress but I need to work a bit more.

@gf712 I believe you will encounter this problem when you start converting undocumented python examples.

karlnapf · 2019-01-14T14:10:31Z

@avramidis we cannot really accept arguments in the watched functions. At least not with the current design, and this was a quite conscious decision. The way around this is to communicate with the classes via put get (to set parameters) and then call helper methods.
If that doesnt help, we might want to think about putting methods into the base interface.

Why does max_string_length need parameters?

avramidis · 2019-01-14T14:19:38Z

OK, thanks for the explanation. The max_string_length doesn't need it but the set_feature_vector

shogun/src/shogun/features/StringFeatures.cpp

Line 248 in f8a120b

    
           template<class ST> void CStringFeatures<ST>::set_feature_vector(SGVector<ST> vector, int32_t num)

karlnapf · 2019-01-14T14:22:34Z

Actually that is a method that we do not want to expose to SWIG, so rather just hide it.
We want feature objects to be immutable eventually, so changing is not allowed, rather construct a new one.

But in cases where we want things like this, it would be best added to the base class.

karlnapf · 2019-01-14T14:27:51Z

examples/meta/src/features/string_char.sg

+#![create_features]
+Features f = string_features(words, enum EAlphabet.RAWBYTE)
+#![create_features]
+


maybe we can add some of the basic things that one would do with string features?
Like extract the string list?

Good idea - I can do that.

I am stuck. I am trying to make get_features work. However, it is overloaded and it is used in many undocumented python examples.

shogun/src/shogun/features/StringFeatures.cpp

Line 1022 in 1c9ac30

template<class ST> SGStringList<ST> CStringFeatures<ST>::get_features()

shogun/src/shogun/features/StringFeatures.cpp

Line 1030 in 1c9ac30

template<class ST> SGString<ST>* CStringFeatures<ST>::get_features(int32_t& num_str, int32_t& max_str_len)

shogun/src/shogun/features/StringFeatures.cpp

Line 1062 in 1c9ac30

template<class ST> void CStringFeatures<ST>::get_features(SGString<ST>** dst, int32_t* num_str)

I don't want to put them in the base interface because they are specific to string features.

You said earlier

The way around this is to communicate with the classes via put get (to set parameters) and then call helper methods.

put and get for watched parameters is fine but what helper methods you mean?

I just spend quite some time trying to register get_features with watch_param. Tricky one!

You will first need to make the method const (both of them as one calls the other). The compiler error you see when trying to register it comes from const, not from overloading (although it looks like that).

Then you will hit another compiler error, which complains about bool shogun::any_detail::compare not matching with the call (among others). This comes from the fact that anything that is ever any'ed in Shogun needs to be comparable and cloneable for SGObject::clone/equals. This hasn't yet been done for SGStringList. You will have to add equals and clone to SGStringList (see SGMatrix/SGVector/ for inspiration). Then those compiler errors should vanish. I don't know what happens then, but it will bring you one step closer to making the get work for the list of string features.

avramidis · 2019-01-14T22:57:48Z

Nice, thanks for that. I have a branch for trying to watch methods with arguments. I was working on that in the weekend. We definitely don't need something like that, right?

…

On Mon, 14 Jan 2019, 22:52 Heiko Strathmann ***@***.*** wrote: ***@***.**** commented on this pull request. ------------------------------ In examples/meta/src/features/string_char.sg <#4423 (comment)> : > @@ -0,0 +1,10 @@ +File words = csv_file("../../data/words.dat") + +#![create_features] +Features f = string_features(words, enum EAlphabet.RAWBYTE) +#![create_features] + I just spend quite some time trying to register get_features with watch_param. Tricky one! You will first need to make the method const (both of them as one calls the other). The compiler error you see when trying to register it comes from const, not from overloading (although it looks like that). Then you will hit another compiler error, which complains about bool shogun::any_detail::compare not matching with the call (among others). This comes from the fact that anything that is ever any'ed in Shogun needs to be comparable and cloneable for SGObject::clone/equals. This hasn't yet been done for SGStringList. You will have to add equals and clone to SGStringList (see SGMatrix/SGVector/ for inspiration). Then those compiler errors should vanish. I don't know what happens then, but it will bring you one step closer to making the get work for the list of string features. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#4423 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AHEjTfED3XnXqoaybFwiI2AxVl-eHts_ks5vDQojgaJpZM4Yw5h2> .

karlnapf · 2019-01-14T22:58:52Z

I think that would be quite hard as then we would need multiple get methods with different number of arguments as well or?

karlnapf · 2019-01-14T22:59:39Z

I also think our new API design doesn't need that (as far as I can see yet)

avramidis · 2019-01-14T23:02:59Z

I was trying to do that with variadic templates. I still get some errors but it can be done I think. I have to dig more into the new API because it seems I have some knowledge gaps.

…

On Mon, 14 Jan 2019, 22:59 Heiko Strathmann ***@***.*** wrote: I also think our new API design doesn't need that (as far as I can see yet) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#4423 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AHEjTUFnpJ_-zHvPtxjV0tPElHtSEBQ6ks5vDQvggaJpZM4Yw5h2> .

karlnapf · 2019-01-14T23:50:52Z

I mean sure it is possible to offer this, exactly using those flexible templates. But it obscures the get API, which quite a cost for what is gained. IF we need such a thing (calling methods by a string, with multiple parameters), then we should add some other function that get.... something like run. But until we don't have to do that, I would be careful getting involved with such things.

BTW the new API is all moving around. We are trying to figure out what is best still.

karlnapf · 2019-01-18T14:32:13Z

Any news here on this?

avramidis · 2019-01-18T14:50:33Z

yes, I am still working on it but I am close. I had to create clone/equals methods for a couple of more classes which caused some unit tests to fail. I fixed those but now it fails for python/java generated examples. I will push again for feedback.

karlnapf · 2019-01-18T14:51:36Z

ah ok. Yeah just push stuff. It is easiest to discuss it here. Might save you some time if devs point out stuff. Shogun is a beast ;)

avramidis · 2019-01-18T15:58:52Z

The runtime error for the python string_char.sg meta example is:

290: Traceback (most recent call last): 290: File "/home/ea461/Workfolder/shogun_project/shogun/build/examples/meta/python/features/string_char.py", line 17, in <module> 290: features = f.get("get_features") 290: File "/home/ea461/Workfolder/shogun_project/shogun/build/src/interfaces/python/shogun.py", line 162, in _internal_get_param 290: raise KeyError("There is no parameter called '{}' in {}".format(name, self.get_name())) 290: KeyError: "There is no parameter called 'get_features' in StringFeatures"

src/shogun/features/StringFeatures.cpp

src/shogun/features/StringFileFeatures.cpp

src/shogun/lib/SGStringList.cpp

src/shogun/lib/SGStringList.h

karlnapf · 2019-01-18T16:05:01Z

tests/unit/features/StringFeatures_unittest.cc

+
+TEST(StringFeaturesTest,equals)
+{
+	SGStringList<char> strings = generateRandomStringData();


we would want a test for SGStringList ... all shogun objects are tested for clone and equals in here

I've seen those. These are tests for constructors without arguments, right?

For now, yes
We might add a version with random content soon

Same here (remove this). I would however love to see a test for clone/equals of SGStringList as those are very low level operations

And we should probably do these tests for all legal template arguments using the googletest magic.
@gf712 can help here

Yup, for this you need to define the types you want to test:

shogun/tests/unit/mathematics/linalg/operations/Eigen3_operations_unittest.cc

Lines 47 to 50 in 13961a1

typedef ::testing::Types<

int8_t, int16_t, uint16_t, int32_t, uint32_t, int64_t, uint64_t, float32_t,

float64_t, floatmax_t, char>

AllTypes;

Associate it to a (empty) class:

shogun/tests/unit/mathematics/linalg/operations/Eigen3_operations_unittest.cc

Lines 26 to 29 in 13961a1

template <typename T>

class LinalgBackendEigenAllTypesTest : public ::testing::Test

{

};

shogun/tests/unit/mathematics/linalg/operations/Eigen3_operations_unittest.cc

Line 59 in 13961a1

TYPED_TEST_CASE(LinalgBackendEigenAllTypesTest, AllTypes);

And then just pass the class to the TYPED_TEST macro

shogun/tests/unit/mathematics/linalg/operations/Eigen3_operations_unittest.cc

Line 64 in 13961a1

TYPED_TEST(LinalgBackendEigenAllTypesTest, SGVector_add)

In the test you can access the type with TypeParam

karlnapf · 2019-01-18T16:09:30Z

it seems weird that this error shows up. Are you sure you are running the same shogun version as the source shows? You could find out via printing all parameter names using f.parameter_names()...if it is not there, then your change hasnt made it into the lib yet

avramidis · 2019-01-29T20:52:26Z

I will work on the SkipImport solution then.

karlnapf · 2019-01-29T20:53:26Z

it is pretty easy to add a blacklist to the logic. You would check if the dictionary contains the blacklist key "ExcludeImport", and then assume you get a list of imports to exclude....
when all the unions are taken in translate.py, you can then remove any blacklisted imports in there
https://github.com/shogun-toolbox/shogun/blob/develop/examples/meta/generator/translate.py#L335

avramidis · 2019-01-29T20:54:47Z

That what I was thinking too. I hope this is the last problem with this PR :)

karlnapf · 2019-01-29T20:55:58Z

if "ExcludeImports" in self.targetDict:
            dependencies.remove(self.targetDict["ExcludeImports"]) # something in these lines, after all union

karlnapf · 2019-01-29T20:56:36Z

Hehehe, you will be surprised ;)

avramidis · 2019-01-29T22:23:31Z

OK the JAVA problem is fixed. Tomorrow I will take a look the octave one.

karlnapf · 2019-01-30T07:21:54Z

Cool! Can you push so we can see what you did in the end?

karlnapf · 2019-01-30T07:22:36Z

For octave can you paste the generated code?

avramidis · 2019-01-30T11:20:19Z

Ok. I pushed the changes. I tested octave on my pc and there are no errors.

src/interfaces/java/swig_typemaps.i

karlnapf · 2019-01-30T12:20:42Z

Cool, just checked the changes, all looks good. I will wait for the CI :)

avramidis · 2019-01-30T12:22:55Z

Great! Thank you!

karlnapf · 2019-01-30T12:53:27Z

@avramidis octave failed. Could you pls post the listing of the char features example?

karlnapf · 2019-01-30T13:00:06Z

Error is explained here

karlnapf · 2019-01-30T13:01:43Z

And here we go: https://github.com/shogun-toolbox/shogun/blob/develop/src/interfaces/octave/swig_typemaps.i#L369

karlnapf · 2019-01-30T13:03:14Z

Have a look at the incoming typemap....looks like we need to write an octave string list typemap to make this work. It is actually quite good that we spotted that as it means one was never ever able to execute string based algorithms from octave ... :/
Let me know if you need help with writing the typemap.

avramidis · 2019-01-30T13:29:31Z

OK no problem. I will make the change.

avramidis · 2019-02-14T13:56:43Z

@karlnapf are the changes ok with you?

karlnapf

Great effort for the typemap!!
Did you do some basic checks in terms of memory errors/leaks and passing string in and out, eg out one in then retrieve it back and ensure it is the same? And all the local variables of the octave program/shogun are still valid?
Then it’s good to be merged :)
We can then later add strings as part of the testing files to ensure results don’t change, but that can be another pr

src/interfaces/octave/swig_typemaps.i

karlnapf · 2019-02-16T08:54:39Z

src/interfaces/octave/swig_typemaps.i

+
+    for (i = 0; i < num_strings; i++) {
+        c(i)=std::string(str[i].string);
+        SG_FREE(str[i].string);


Does this maybe cause problems? The original string is destroyed?

Correct. I copied these from the other languages interfaces. I removed them.

karlnapf · 2019-02-20T08:15:05Z

Very nice work! This now allows to port quite a few more examples

karlnapf reviewed Nov 23, 2018

View reviewed changes

examples/meta/generator/targets/cpp.json Show resolved Hide resolved

examples/meta/generator/targets/cpp.json Outdated Show resolved Hide resolved

src/shogun/features/StringFeatures.h Outdated Show resolved Hide resolved

avramidis force-pushed the feature/features-string-char-meta-example branch from 5f6cc7a to be01de6 Compare December 13, 2018 21:20

avramidis mentioned this pull request Dec 14, 2018

Use of auto in cpp meta examples #4436

Closed

karlnapf reviewed Jan 14, 2019

View reviewed changes

avramidis force-pushed the feature/features-string-char-meta-example branch from be01de6 to 71a7f3e Compare January 18, 2019 15:01