Check type of Any for casting #4426

gf712 · 2018-11-29T16:24:25Z

To cast Any back to its original type we need to check each type used in shogun before casting.
I implemented a function that does this check and then executes a lambda when it finds the right type.
It still needs some work:

handle errors
add more type maps
maybe add some sort of check for the lambda, i.e. does it have the expected signature?
should it be void rather than return int? The int was just initially used for debugging.

Example usage:

#include <iostream>
#include <shogun/base/init.h>
#include <shogun/io/SGIO.h>
#include <shogun/lib/type_case.h>

using namespace shogun;

int main() {

	init_shogun();

	int a = 2.0;
	Any any = make_any(a);

	// declare a lambda that is executed when the original type is found
	// needs this signature
	auto found_it_lambda = [](auto value) {
		std::cout << "It's a " << demangled_type<decltype(value)>() << std::endl;
		std::cout << "Value " << value << std::endl;
	};

	try {
		sg_any_dispatch(any, sg_all_types, found_it_lambda);
	}
	catch (ShogunException &msg) {
		std::cout << msg.what() << std::endl;
	}

	try {
		sg_any_dispatch(any, sg_real_types, found_it_lambda);
	}
	catch (ShogunException &m) {
		std::cout << m.what() << std::endl;
	}
}

Expected output:

It's a int
Value 2
[ERROR] In file /Users/ghoben/anaconda3/include/shogun/lib/type_case.h line 197: Type int is not part of <long double, double, float>

karlnapf · 2018-11-29T19:12:41Z

@lisitsyn check this out

karlnapf · 2018-11-29T19:14:17Z

src/shogun/lib/type_case.h

+	{
+		if (type == sg_primitive_type<typename T1::Head>::value)
+		{
+			typename T1::Head i;


what about a better name than i

Also the typenames could me more meaningful

OK! I just gave them the most standard names because I'm not very innovative with variable names..

Also should change the file name.. It's called type_case because the first implementation used a switch..

karlnapf · 2018-11-29T19:14:37Z

src/shogun/lib/type_case.h

+
+	template <typename T1, typename T2>
+	typename std::enable_if<std::is_same<T1, Types0>::value, int>::type
+	recursive_type_finder(const Any& any, TYPE type, T2 func)


what about just type_finder? Or even something else?

karlnapf · 2018-11-29T19:14:58Z

src/shogun/lib/type_case.h

+	}
+
+	template <typename T>
+	int type_finder(const Any& any, typemap& typesmap, T func)


for_each_type? not appropriate anymore?

Yes, that could work. Btw do you know why it is considered better to pass a lambda as a rvalue reference rather than as a copy, i.e. T&& func rather than T func

ehm ....it rings some bell, but I can't remember right now... stackoverflow ;)

Seems to be better to use rvalue reference if the lambda call is performed out of scope. But I don't see how that would happen, as the lambda never moves ownership. I cant tell if there is a downside to always use T&& func.

karlnapf · 2018-11-29T19:15:43Z

src/shogun/lib/type_case.h

+{
+};
+
+#define SG_PRIMITIVE_TYPE(T, ptype)                                            \


make sure to udef all the macros after using them. We don't want to pollute the scope too much

karlnapf · 2018-11-29T19:18:07Z

src/shogun/lib/type_case.h

+// the head of the list returns the type
+typedef Types<
+    bool, char, int8_t, uint8_t, int16_t, uint16_t, int32_t, uint32_t, int64_t,
+    uint64_t, float32_t, float64_t, floatmax_t>::type AllTypes;


sg_types
SG_TYPES
?
Also, those are only scalar types. What about adding a matrix type to start seeing what happens in those cases?
We will also need std::vector and friends.....which might show the issues with this approach: combinatorial explosions.
I guess we could argue that the number of types are limited.

karlnapf · 2018-11-29T19:18:37Z

src/shogun/lib/type_case.h

+	// here we make types switchable
+	enum class TYPE
+	{
+		PT_BOOL = 1,


the PT_ prefix comes from somewhere else?

yes, that comes from shogun/tests/unit/base/SGObjectAll_unittest.cc

i see, what about somehow re-using those from DataType.h

Ah seem to be the same! But will have to address the containers..

Well we don't want to support all the containers. Also the container definitions are part of the old type system and probably will be removed soon (nobody needs strings of SGVectors).
What is more likely is that we support matrix/vector of the ptypes, and then strings...but let's do that later when it is needed.

karlnapf · 2018-11-29T19:18:52Z

src/shogun/lib/type_case.h

+		PT_FLOAT32 = 11,
+		PT_FLOAT64 = 12,
+		PT_FLOATMAX = 13,
+		PT_SGOBJECT = 14,


not sure these are still supposed to be in here?

uuuh, actually we might need to add all base types here as well....sigh
@lisitsyn any thoughts?

karlnapf

This is definitely a big step forward in dispatching any types.
I feel that we cannot support this direction for any type in shogun, as this would escalate the number of types. Instead, we want to define a carefully selected subset of types for which this feature could be used (e.g. all scalar types and histograms for tensorboard logging, generally all model parameters).
We can iteratively extend this after a first prototype works....

gf712 · 2018-11-30T10:28:40Z

I agree, you need to be sensible about what types you want to check for. Otherwise you do end up spending more and more time looking for the type. Also all types take the same amount of time to be searched for, i.e. bool type would take the same time as complex128_t. However, a narrower search space reduces the search time.

karlnapf

F

karlnapf · 2018-11-30T11:13:52Z

src/shogun/io/TBOutputFormat.cpp

@@ -106,6 +107,13 @@ tensorflow::Event TBOutputFormat::convert_scalar(
 		    "Unsupported type %s", value.first.get_value().type_info().name());
 	}

+	auto write_summary = [&](auto type) {


Question, could we explicit here with the variables passed on as reference?

Do you mean something like:

auto write_summary = [value, summaryValue](auto type) {summaryValue->set_simple_value( any_cast<decltype(type)>(value.first.get_value())); };

That should be safer!

Or would it be:

auto write_summary = [value, this](auto type) {this->summaryValue->set_simple_value( any_cast<decltype(type)>(value.first.get_value())); };

like that yes, so that callers are forced to think about the scope

btw it seems like it should be like this in C++ 14 (including this is C++11):

auto write_summary = [&summaryValue=summaryValue, &value](auto type) { summaryValue->set_simple_value( any_cast<decltype(type)>(value.first.get_value())); };

https://stackoverflow.com/a/42029220

ok great, this is quite redundant anyways, I like the C++ updates :)

Yup, now just need to come up with a way to make sure people use this signature...
Would it be worth having some checks (not sure how)? Or just add somewhere in the documentation?

I think it is fine to let people look at examples and then use the same style. Documentation just outdates, and this is only for developers (who know how to browse the codebase) anyways.
What happens if a different signature is passed. Compile error I guess so that would be fine.

karlnapf · 2018-11-30T11:14:53Z

src/shogun/lib/type_case.h

+		{
+			typename TypeList::Head temporary_type_holder;
+			func(temporary_type_holder);
+			return 1;


Is there a specific reason for 0 and 1? As opposed to something else?

No, could be a boolean, or could be a void function. This is just a way to capture errors, like in my example above we could do this:

if (!for_each_type(any, all_types, found_it_lambda)) { std::cout << "FAILED" << std::endl; }

Or could just throw an error from within the function if the type isn't found? Or allow a custom lambda to be executed in that situation?

mmmh, we could throw an exception (recently added customized exceptions might be worth checking this out). If not, a boolean might be better than the int

question is whether there will be situations where no type is found but the caller doensnt know that....
If this does not make sense, exception would be best (with a nice error string) imo

I am trying out SG_ERROR, but it won't work here because it seems to only work with classes that inherit from CSObject.

@vinx13 could you advise how to best use your new expections?

@gf712 try SG_SERROR for static contexts

Generally, we want to move towards better exceptions, with types. But this is especially true for errors that can be caused by users. This is more an internal error that is caused by a developer...so maybe the SG_SERROR is ok as well

Use SG_THROW or SG_STHROW to throw an exception with custom type.
The first argument is the type of the exception. The rest of the arguments are the same as SG_ERROR
e.g.
SG_THROW(ShogunException, "...")

Ah cool! Thanks, works as expected!

Ah actually no, this gives me a segmentation fault:

SG_SERROR("Unsupported type %s", any.type_info().name());

which I didn't have when I did this:

char buffer[256]; sprintf(buffer, "Unsupported type %s", any.type_info().name()); auto msg = std::string(buffer); throw ShogunException(msg);

ah nvm, I just need to add init_shogun() at the start of my example code!

karlnapf · 2018-11-30T11:15:17Z

src/shogun/lib/type_case.h

+			ADD_TYPE_TO_MAP(float64_t , TYPE::PT_FLOAT64)
+			ADD_TYPE_TO_MAP(floatmax_t , TYPE::PT_FLOATMAX)
+	};
+	typemap non_integer_types = {


Nice this will be usefil

karlnapf · 2018-12-01T10:48:02Z

src/shogun/io/TBOutputFormat.cpp

+		    any_cast<decltype(type)>(value.first.get_value()));
+	};
+
+	for_each_type(value.first.get_value(), all_types, write_summary);


sorry one more thing. Could we make the naming in a way that it is clear that this is a shogun utility function.
Like adding a prefix sg_ to all functions and global variances that have to do with this

OK! Btw, do you have any rule when to put or leave out functions/typedefs/enums from the namespace?

minimal evasion, so only make something as visible/complex as needed, not more

karlnapf · 2018-12-01T10:49:01Z

src/shogun/lib/type_case.h

+{
+	typedef Types<
+	    bool, char, int8_t, uint8_t, int16_t, uint16_t, int32_t, uint32_t,
+	    int64_t, uint64_t, float32_t, float64_t, floatmax_t>::type SG_TYPES;


so this collects all types that we support going from any to compile time type right?
So if we added SGMatrix<float64_t>, it would be added here or?

Yes, it should be added there. And then in the enum class TYPE and use SG_PRIMITIVE_TYPE to map the type to the enum and then to the appropriate map!

can we have one vector/matrix type for illustration?

Yes, I tried it out. It can cause some issues though.
Let's say we add SGVector<float32_t> to the sg_all_types map. Everything works fine. But then if we use it in the TBOutputFormat.cpp we get a compile error, because the compiler tries to convert SGVector<float32_t> to float, because of the signature of summaryValue->set_simple_value(float) (error: no viable conversion from 'shogun::SGVector<float>' to 'float').
We will need to keep a second TypeList with SGVector and probably another one for SGMatrix. And then either have different sg_for_each_*_type functions, or come up with something fancier.. Working on it!

this means the caller needs to know what container type she is dealing with ... mmh
Ok curious what you come up with

No I think it is good to offer functions to deal with vector/matrix/std::vector separately... we do this specialization inside Any as well (see equals/clone)

Ah ok! I do think I found a solution. Can basically have different parameters in the sg_for_each_type to execute lambdas for specific datatypes, and then with some SFINAE magic can execute the right lambda or throw an error (or do nothing). Could a have a function signature like this:

template <typename Lambda1 = std::nullptr_t, typename Lambda2 = std::nullptr_t, typename Lambda3 = std::nullptr_t> void sg_for_each_type(const Any& any, const typemap& typesmap, Lambda1 scalar_func = nullptr, Lambda2 vector_func = nullptr, Lambda3 matrix_func = nullptr)

I got this logic working. Basically can pass a lambda to be executed if it is a scalar, SGVector or SGMatrix. If the type that is found is any of those but there is no lambda defined for that type I can throw an error, warning or even nothing (but would be hard to debug).. This could be good if you expect Any to have either of these types and can handle everything with a single call!

And then if the type wasn't registered we get the ShogunException("Unsupported type %s") at runtime. Might be worth changing it to a static_assert to get a compile time error?

++ for static_assert, always nicer

karlnapf · 2018-12-01T10:50:07Z

src/shogun/lib/type_case.h

+#define ADD_TYPE_TO_MAP(TYPENAME, TYPE_ENUM)                                   \
+	{std::type_index(typeid(TYPENAME)), TYPE_ENUM},
+
+	typemap all_types = {


sg_ prefixes would be good I think

You mean sg_all_types right? (as opposed to the name of the macro)

karlnapf · 2018-12-01T10:54:41Z

Expected output:

It's a int
Value 2
[ERROR] In file /Users/ghoben/anaconda3/include/shogun/lib/type_case.h line 161: Type i is not part of the given typemap

Just looking at the output. The error message is a bit cryptic. It should state "Type int32_t is not part of <float32_t, float64_t, float128_t>" or something in those lines

gf712 · 2018-12-03T10:36:51Z

Without much more code I can get the message Type int is not part of <long double, double, float>
To get "Type int32_t is not part of <float32_t, float64_t, float128_t>" we would have to map the demangled strings to those strings, i.e. {"long double": "float128_t"}

karlnapf · 2018-12-03T13:02:20Z

Without much more code I can get the message Type int is not part of <long double, double, float>
To get "Type int32_t is not part of <float32_t, float64_t, float128_t>" we would have to map the demangled strings to those strings, i.e. {"long double": "float128_t"}

That is fine actually, no neep to map

karlnapf · 2018-12-03T16:10:21Z

src/shogun/lib/type_case.h

@@ -75,10 +75,49 @@ namespace shogun
 {
 	typedef std::unordered_map<std::type_index, TYPE> typemap;

+	namespace type_internal
+	{
+		std::string demangled_type(const char* name)


wait, didn't we have a function like this somewhere already?

yes, it is similar, but it is a templated function that requires the type. Here we don't have the type, but have the std::type_info::name instead (and we cant infer type from that), so I needed this custom function that I hid in this namespace.

any way to re-use some redundant code?

especially the guard/macro bit....and only if it doesnt cause more trouble than it solves

I will give it a go! I can add both implementations in Any.h

for example.
It might even go somewher else, as I dont think it is any specific

I put it in Any.cpp as I was getting a duplicate symbol linker error.

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

src/shogun/lib/type_case.h

-		type_finder<SG_TYPES>(any, type, func);
+			    "Type %s is not part of %s",
+			    type_internal::demangled_type(any.type_info().name()).c_str(),
+			    type_internal::print_map(typesmap).c_str())


ah sorry, we actually need it here, nevermind my above comment

karlnapf · 2018-12-03T16:12:51Z

src/shogun/lib/type_case.h

+	}
+
+	template <typename Lambda>
+	void sg_for_each_type(const Any& any, typemap& typesmap, Lambda func)


one thing: could you write a clear docstring for this function? So that it will be immediately clear to other devs how to use it (not so much how it works)

OK! Should I add information about the signature? I am assuming it will remain as it is now?

yes doxygen docstrings have a descr. for the function and then one field for every parameter

/** This is the description, describes the behaviour. @param foo first parameter, and potential details @param bar second parameter @return the result */

Oh I meant if I should write the signature for the lambda in the description, sorry!

Yes I think this is a good thing to document (we will see whether it outdates)

karlnapf · 2018-12-03T16:13:52Z

src/shogun/lib/type_case.h

+	template <typename TypeList, typename Lambda>
+	typename std::enable_if<
+	    (not std::is_same<TypeList, Types0>::value), void>::type
+	sg_type_finder(const Any& any, TYPE type, Lambda func)


is this function used anywhere else, apart from sg_each_type? If not, then we should hide it from the global scope and only define/use it locally in the dispatcher

Good point, I can move it to the other namespace I created? Btw, should the other namespace be nested in shogun (i.e. shogun::type_internal), or completely separate?

yes move it to the other.
IDK whether they should be nested or not .... I guess so, would there be any case where we would use the latter on its own?

OK! No, I doubt it, it's very specific.

karlnapf · 2018-12-03T16:17:44Z

src/shogun/io/TBOutputFormat.cpp

+		    any_cast<decltype(type)>(value.first.get_value()));
+	};
+
+	sg_for_each_type(value.first.get_value(), sg_all_types, write_summary);


I think now that this works nicely and looks good, we should also have some minimal unit testing that ensures very basic functionality of the for_each thing. One test that makes sure dispatching works, one that checks failure cases caused by ignorant devs, and maybe one for corner cases .?

All right! I am guessing those tests fit in Any_unittest.cc?

yep, or an own test for the type_finger (whatever the name is)
To add tests, just add another cpp file and re-run cmake to detect it

karlnapf

It is really getting there.
I made a few more comments but it gets quite marginal now ... so we should soon merge this

karlnapf · 2018-12-04T23:09:33Z

src/shogun/io/TBOutputFormat.cpp

@@ -112,7 +112,7 @@ tensorflow::Event TBOutputFormat::convert_scalar(
 		    any_cast<decltype(type)>(value.first.get_value()));
 	};

-	sg_for_each_type(value.first.get_value(), sg_all_types, write_summary);
+	sg_for_each_scalar_type(value.first.get_value(), sg_all_types, write_summary);


mmh this name is a bit of a monster now ;)
I am thinking loud ... sg_dispatch_any, sg_dispatch_any_vector, sg_dispatch_any_matrix, sg_dispatch_any_stl
Don't know if that is even better

Including "any" in the function name might make it easier to find the function in docs! So something along those lines would be better.

ah ... I already wrote something earlier....
sg_any_dispatch is my fav atm ... but as said this might change

karlnapf · 2018-12-04T23:10:26Z

src/shogun/lib/type_case.h

@@ -38,8 +42,10 @@ namespace shogun
 		PT_FLOATMAX = 13,
 		PT_SGOBJECT = 14,
 		PT_COMPLEX128 = 15,
-		PT_UNDEFINED = 16
+		PT_SGVECTOR_FLOAT32 = 16,


PT stands for "primitive type" in the old parameter framework. So keeping that in here doesnt make sense, maybe we need a new prefix?

Could just leave without a prefix? Otherwise just use SG_ instead?

what about T_

Yup, can use that

gf712 · 2018-12-05T09:50:16Z

Getting this error from CI for the lint job:

2018-12-04T13:38:11.4740116Z ##[error]/usr/bin/docker: invalid reference format.
2018-12-04T13:38:11.4753526Z ##[error]See '/usr/bin/docker run --help'.
2018-12-04T13:38:11.4754489Z ##[error]/usr/bin/docker failed with return code: 125

Is that an issue with the script or is the format not correct in my code?

karlnapf

Awesome that this now works for containers...
The only thing that concerns me is the API, I need to think a bit about how to avoid the default argument nullptr trick.
But it is good to see what happens if this is scaled up a bit.

karlnapf · 2018-12-06T21:07:57Z

src/shogun/lib/type_case.h

+		bool, char, int8_t, uint8_t, int16_t, uint16_t, int32_t, uint32_t,
+		int64_t, uint64_t, float32_t, float64_t, floatmax_t, SGVector<int32_t>,
+		SGVector<int64_t>, SGVector<float32_t>, SGVector<float64_t>,
+		SGVector<floatmax_t>, SGMatrix<int32_t>, SGMatrix<int64_t>,


those should suffice for now, so nice :)

Do we need std::vector too?

Let's think. The feature of going from any to typed will be used in

logging (only model parameters, numerical stuff)

modelselecton (again, mostly model parameters, here we will need SGObject as well ...)

not sure what else?
So with all that in mind, I think those types should be ok. And we can extend on request .... like done for any

karlnapf · 2018-12-06T21:08:58Z

src/shogun/lib/type_case.h

@@ -38,8 +42,10 @@ namespace shogun
 		PT_FLOATMAX = 13,
 		PT_SGOBJECT = 14,
 		PT_COMPLEX128 = 15,
-		PT_UNDEFINED = 16
+		PT_SGVECTOR_FLOAT32 = 16,


what about T_

karlnapf · 2018-12-06T21:09:30Z

src/shogun/lib/type_case.h

+
+	enum class CONTAINER_TYPE
+	{
+		SCALAR = 1,


karlnapf · 2018-12-06T21:10:16Z

src/shogun/lib/type_case.h

+	{
+		SCALAR = 1,
+		VECTOR = 2,
+		MATRIX = 3


if there is another "container type" enum, I don't really get why we then also need to state the containers here?
Having both seems redundant...? Maybe I just don't get it

is this used anywhere?

No, sorry, I was going to try something else and then gave up and forgot to delete this

karlnapf · 2018-12-06T21:11:09Z

src/shogun/lib/type_case.h

+SG_PRIMITIVE_TYPE(SGMatrix<float64_t>, TYPE::PT_SGMATRIX_FLOAT64)
+SG_PRIMITIVE_TYPE(SGMatrix<floatmax_t>, TYPE::PT_SGMATRIX_FLOATMAX)
+SG_PRIMITIVE_TYPE(SGMatrix<int32_t>, TYPE::PT_SGMATRIX_INT32)
+SG_PRIMITIVE_TYPE(SGMatrix<int64_t>, TYPE::PT_SGMATRIX_INT64)


this is not nice and quite long....
BUT the good thing is that we (shogun framework devs) do this once, and not people who write higher level framework code.

I know.. The only other time that it might be used is if someone wants to create a custom map. Do you mean the list is long or the names?

no just the big stack. But it is OK, as it is better than before

karlnapf · 2018-12-06T21:13:35Z

src/shogun/lib/type_case.h

+			T value, ScalarLambdaT scalar_func, VectorLambdaT vector_func,
+			MatrixLambdaT matrix_func)
+		{
+			SG_SWARNING(


I feel this should not be a runtime warning?

I have been thinking that a lot of these things should be static asserts to make debugging easier... If everything compiles any developer might become more complacent...

We could check the signature of the function to see if it is void and has one parameter at compile time.

Got something like this working:

static_assert(std::is_same< check_return_type<typename TypeList::Head, ScalarLambdaT, VectorLambdaT, MatrixLambdaT>, ok>::value, "All lambda definitions must be void and have the signature 'void f(auto type)'");

Not the most useful feature, but means we can write a test where the function signature is incorrect without compilation errors for the test! And gives the developer a "gentler" compile error.

I like such static assertions a lot!

Alright, got 1 static assertion working! I didn't work on anything else yet as this took a while to figure out. I also wanted to add a test for static_assert with gtest, and that wasn't easy but now its working! :D

Should I replace the warning with an exception? The idea of the warning was that there could be situations where no lambda implementation exists of the expected type on purpose and an exception could abort a ML run without need.

Alright, got 1 static assertion working! I didn't work on anything else yet as this took a while to figure out. I also wanted to add a test for static_assert with gtest, and that wasn't easy but now its working! :D

nice one!

Should I replace the warning with an exception? The idea of the warning was that there could be situations where no lambda implementation exists of the expected type on purpose and an exception could abort a ML run without need.

I would say yes, with an appropriate string describing what is wrong

karlnapf · 2018-12-06T21:15:44Z

src/shogun/io/TBOutputFormat.cpp

+		    any_cast<decltype(type)>(value.first.get_value()));
+	};
+
+	sg_for_each_type(value.first.get_value(), sg_all_types, write_summary);


since you have the vector thingi now in there, maybe also refactor the histogram bit?

I was thinking about that, but it uses std::vector instead of SGVector? Is there some implicit casting going on?

mmh phew ...
I would like to think it is possible to refactor this to SGVector?
On the other hand, it might be nice to support std:: structures as well

I think the conversion to std::vector is necessary, because tensor board doesn't know about SGVector, like in this case:

shogun/src/shogun/io/TBOutputFormat.cpp

Lines 125 to 135 in 2f0b9cf

if (value.first.get_value().type_info().hash_code() ==

typeid(std::vector<int8_t>).hash_code())

{

tensorflow::histogram::Histogram h;

tensorflow::HistogramProto* hp = new tensorflow::HistogramProto();

auto v = any_cast<std::vector<int8_t>>(value.first.get_value());

for (auto value_v : v)

h.Add(value_v);

h.EncodeToProto(hp, true);

summaryValue->set_allocated_histo(hp);

}

karlnapf · 2018-12-06T21:16:27Z

src/shogun/io/TBOutputFormat.cpp

+		    any_cast<decltype(type)>(value.first.get_value()));
+	};
+
+	sg_for_each_type(value.first.get_value(), sg_all_types, write_summary);


I think you might kill me, but maybe "dispatch" should be part of the function name? sg_any_dispatch comes to my mind now.... but not sure what is better. What are your toughts?

Yes that sounds good! Was just waiting for a final decision before changing the name!

lol ... let's hope this is the last :D

karlnapf · 2018-12-06T21:18:34Z

tests/unit/lib/type_case_unittest.cc

+TEST(Type_case, positional_lambdas)
+{
+	float32_t a_scalar = 42.0;
+	auto a_vector = SGVector<float32_t>({a_scalar});


minor
SGVector<float32_t> a_vector = {a_scalar} is a tiny bit shorter ;)

karlnapf · 2018-12-06T21:20:03Z

tests/unit/lib/type_case_unittest.cc

+	sg_for_each_type(any_vector, sg_all_types, nullptr, f_vector);
+	EXPECT_EQ(counter, 2);
+
+	sg_for_each_type(any_matrix, sg_all_types, nullptr, nullptr, f_matrix);


this API is not the most elegant....need to discuss this

Hmmm ok! Can have three different functions, it could make my life easier when specialising the helper function templates

I wonder: Can't this be implicitly be done by forcing the lambda to have a different signature (including SGVector/matrix) ....
If not, I think having three functions is the best bet, at least API wise. They all could call the function you wrote here internally to avoid redundant code

I think that could be possible, assuming that the compiler can implicitly convert SGVector and SGMatrix specialisations, right? Like in the summaryValue->set_simple_value(float value), I think all scalars can be interpreted as float right?

Btw seems like std::is_scalar is quite a broad definition, will have to find a more strict definition like std::is_arithmetic (potentially combined with a std::decay)

I think that could be possible, assuming that the compiler can implicitly convert SGVector and SGMatrix specialisations, right?
What do you mean by that? BTW not sure if that helps, but It is definitely possible to extract inner types from templates, see this ugly thing for example

Like in the summaryValue->set_simple_value(float value), I think all scalars can be interpreted as float right?
That is true, but the compiler will not implicitly convert float to SGVector

Btw seems like std::is_scalar is quite a broad definition, will have to find a more strict definition like std::is_arithmetic (potentially combined with a std::decay).
Yes definitely, I would have a typedef for that that checks, see here

I think that could be possible, assuming that the compiler can implicitly convert SGVector and SGMatrix specialisations, right?
What do you mean by that?

Basically if we have a lambda that takes SGVector<float32_t> it doesn't throw a compiler error when it sees the type SGVector<float64_t> and instead thinks that it can be converted.

karlnapf · 2018-12-06T21:23:52Z

Getting this error from CI for the lint job:
2018-12-04T13:38:11.4740116Z ##[error]/usr/bin/docker: invalid reference format.
2018-12-04T13:38:11.4753526Z ##[error]See '/usr/bin/docker run --help'.
2018-12-04T13:38:11.4754489Z ##[error]/usr/bin/docker failed with return code: 125
Is that an issue with the script or is the format not correct in my code?

I think @vigsterkr is working on CI stuff atm, so it might behave weirdly ...

Added a static_assert as a first example, but could add some more to make sure any developer gets a nice message at compile time. This is largely based on https://channel9.msdn.com/Events/CPP/CppCon-2016/CppCon-2016-Roland-Bock-How-To-Test-staticassert and could probably cleaned up a bit more!

* changed sg_for_each_type to sg_any_dispatch * changed PT_ prefix to T_ * changed util function names

* this is not the final version of the PR but we can revert to here and have a stable API * added more decltype to enable multiple static assertions from more nested functions, i.e. check type of lambda param for 1 lambda API

* added same logic to SGVector and SGMatrix to return false if type hasn't been "registered" * renamed scalar to primitive

all we needed were variadic templates! No need for all the possible definitions like in gtest!

gf712 · 2018-12-14T11:48:45Z

OK, should be good to go now!

karlnapf · 2018-12-18T11:30:13Z

src/shogun/lib/any.h


 namespace shogun
 {
+	std::string demangled_type_helper(const char* name);


shall we maybe hide this one from outside?

karlnapf · 2018-12-18T11:31:27Z

I somehow thought I merged this already? Shall I, or was something still missing?

gf712 · 2018-12-18T15:19:58Z

I think everything is in place and working! It's only a first iteration anyway.

karlnapf · 2018-12-19T18:51:13Z

Ok shall I then? Just had another look and this is definitely something to use for a start

gf712 · 2018-12-20T08:27:39Z

Yes, and then when shogun supports c++17 we can clean this up with some if constexpr

karlnapf · 2018-12-20T08:49:27Z

Ok let’s watch ci. There might be compiler issues on some platforms with this modern stuff

karlnapf · 2018-12-20T08:49:39Z

Awesome!

gf712 · 2018-12-20T08:53:20Z

OK! There seem to be some issues with msvc, like #4443.. But it might be an old bug in the compiler that has been fixed in more recent versions.

gf712 · 2018-12-20T09:46:11Z

CI seems to be failing during testing but due to double free or corruption in other tests..?

karlnapf · 2018-12-20T10:27:47Z

That might be side effects from the changes. Keep in mind all tests are in one single executable and share memory so introducing memory errors in a test can make others fail, or adding a test could reveal existing memory problems due to layout changes

gf712 · 2018-12-20T10:32:31Z

OK, I'll investigate, but I can't reproduce these issues locally

* list to keep track of typename * file adapted from gtest * added type_finder to check find Any type type_finder has a parameter to pass a lambda that is then called when the correct type is passed * added init_types to populate type identifiers when shogun is started * example of how type_finder could be used * fixed spacing with clang-format * moved to header only * this makes it compatible with existing code * maps are now defined statically * cleaned up code * removed header include from init * removed extern from map declaration as it is statically declared now * added undef for local macros * removed functions that are not required * changed names of function, typedefs and typenames changed these to more intuitive names * added more type maps * explicitly capture references in lambda * for_each_type is now a void function added SG_SERROR macro that throws a ShogunException * added sg_ prefix to utility functions and definitions * improved error messages * typenames can be interpreted more easily * code refactor * moved demangled_string with const char* parameter to any.cpp * moved helper functions to type_internal scope * added SGVector<float32> had to add two new functions, one for scalar and another one for vectors * sg_check_each_type accepts scalar, SGVector or SGMatrix * up to 3 lambdas can be passed for each type * added maps for containers * added some simple tests * improved typenames and fixed indentation * static_assert and test Added a static_assert as a first example, but could add some more to make sure any developer gets a nice message at compile time. This is largely based on https://channel9.msdn.com/Events/CPP/CppCon-2016/CppCon-2016-Roland-Bock-How-To-Test-staticassert and could probably cleaned up a bit more! * fixed formatting * minor refactor * changed sg_for_each_type to sg_any_dispatch * changed PT_ prefix to T_ * changed util function names * last round clean up of 3 lambda API * this is not the final version of the PR but we can revert to here and have a stable API * added more decltype to enable multiple static assertions from more nested functions, i.e. check type of lambda param for 1 lambda API * final cleanup * added is_sg_primitive to replace std::is_scalar * added same logic to SGVector and SGMatrix to return false if type hasn't been "registered" * renamed scalar to primitive * simplified Types struct all we needed were variadic templates! No need for all the possible definitions like in gtest! * dropped superfluous typename * hid demangled_type_helper in any_detail namespace * fixed bug when HAVE_CXA_DEMANGLE is not defined

karlnapf reviewed Nov 29, 2018

View reviewed changes

karlnapf reviewed Nov 30, 2018

View reviewed changes

karlnapf reviewed Dec 1, 2018

View reviewed changes

karlnapf closed this Dec 3, 2018

karlnapf reopened this Dec 3, 2018

karlnapf reviewed Dec 3, 2018

View reviewed changes

karlnapf reviewed Dec 4, 2018

View reviewed changes

karlnapf reviewed Dec 6, 2018

View reviewed changes

Gil added 8 commits December 13, 2018 14:21

added some simple tests

9b11694

improved typenames and fixed indentation

685442a

fixed formatting

8ddd574

minor refactor

d1ab3e4

* changed sg_for_each_type to sg_any_dispatch * changed PT_ prefix to T_ * changed util function names

last round clean up of 3 lambda API

3802e4f

* this is not the final version of the PR but we can revert to here and have a stable API * added more decltype to enable multiple static assertions from more nested functions, i.e. check type of lambda param for 1 lambda API

final cleanup

71187ff

added is_sg_primitive to replace std::is_scalar

a001a87

* added same logic to SGVector and SGMatrix to return false if type hasn't been "registered" * renamed scalar to primitive

gf712 force-pushed the feature/check_type_macros branch from c552c45 to a001a87 Compare December 13, 2018 14:53

simplified Types struct

42b8ca9

all we needed were variadic templates! No need for all the possible definitions like in gtest!

avramidis mentioned this pull request Dec 17, 2018

Removed some uses of m_model_selection_parameters [WIP] #4440

Closed

dropped superfluous typename

f44b981

karlnapf reviewed Dec 18, 2018

View reviewed changes

Gil added 2 commits December 18, 2018 17:14

hid demangled_type_helper in any_detail namespace

4a2ec52

fixed bug when HAVE_CXA_DEMANGLE is not defined

99ef50a

karlnapf merged commit 98f0ac0 into shogun-toolbox:develop Dec 20, 2018

gf712 deleted the feature/check_type_macros branch January 15, 2019 11:16

	if (value.first.get_value().type_info().hash_code() ==
	typeid(std::vector<int8_t>).hash_code())
	{
	tensorflow::histogram::Histogram h;
	tensorflow::HistogramProto* hp = new tensorflow::HistogramProto();
	auto v = any_cast<std::vector<int8_t>>(value.first.get_value());
	for (auto value_v : v)
	h.Add(value_v);
	h.EncodeToProto(hp, true);
	summaryValue->set_allocated_histo(hp);
	}

Check type of Any for casting #4426

Check type of Any for casting #4426

Conversation

gf712 commented Nov 29, 2018 • edited

karlnapf commented Nov 29, 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

karlnapf left a comment

Choose a reason for hiding this comment

gf712 commented Nov 30, 2018

karlnapf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gf712 Nov 30, 2018 • 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

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

karlnapf commented Dec 1, 2018

gf712 commented Dec 3, 2018

karlnapf commented Dec 3, 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

karlnapf Dec 3, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gf712 commented Nov 29, 2018 •

edited

gf712 Nov 30, 2018 •

edited

karlnapf Dec 3, 2018 •

edited