Feature/equals with tags #4069

karlnapf · 2018-01-05T18:33:31Z

@lisitsyn This almost works, and side-steps a lot of older code (will delete later).

One thing that is quite annoying is "old" parameter registrations are not covered by tags. See e.g.

m_parameters->add_vector(&m_array.array, &m_array.current_num_elements, "array",
			"Memory for dynamic array.");

in CDynamicObjectArray. This causes some tests to fail when using equals via tags. I am on it, but let me know if you have a quick fix for things like the above (reason SG_ADD is not used is since only a smaller part of the array is registered...)

karlnapf · 2018-01-05T18:35:13Z

src/shogun/lib/any.h

@@ -153,7 +153,12 @@ namespace shogun
 		auto compare_impl(maybe_most_important, T* lhs, T* rhs)
 		    -> decltype(lhs->equals(rhs))
 		{
-			return lhs->equals(rhs);
+			if (lhs && rhs)


@lisitsyn let me know if this is the right place to add those checks

Yeah that's a good catch.

karlnapf · 2018-01-05T18:46:05Z

src/shogun/base/AnyParameter.h

@@ -88,6 +88,16 @@ namespace shogun
 			return m_properties;
 		}

+		inline bool operator==(const AnyParameter& other)
+		{
+			return m_value == other.get_value();


good? not good?

Probably ok. Maybe needs a comment that it doesn't consider properties.

karlnapf · 2018-01-05T18:46:12Z

src/shogun/base/SGObject.cpp

@@ -28,6 +28,7 @@

 #include <stdlib.h>
 #include <stdio.h>
+#include <typeinfo>


good? not good?

Was it missed?

karlnapf · 2018-01-05T18:46:23Z

src/shogun/base/SGObject.cpp

@@ -820,7 +743,7 @@ AnyParameter CSGObject::get_parameter(const BaseTag& _tag) const
 	const auto& parameter = self->get(_tag);
 	if (parameter.get_value().empty())
 	{
-		SG_ERROR("There is no parameter called \"%s\" in %s",
+		SG_ERROR("There is no parameter called \"%s\" in %s\n",


karlnapf · 2018-01-05T18:46:41Z

src/shogun/base/SGObject.cpp

+		return false;
+	}
+
+	/* Assumption: can use SGObject::get_name to distinguish types */


good? not good?

karlnapf · 2018-01-05T18:47:10Z

src/shogun/base/SGObject.cpp

+
+		SG_SDEBUG("Comparing parameter %s::%s.\n", this->get_name(),
+				tag.name().c_str());
+		if (own != given)


all the magic happens in this single line, awesome!

karlnapf · 2018-01-05T18:47:56Z

src/shogun/base/SGObject.cpp

+				std::stringstream ss;
+				std::unique_ptr<AnyVisitor> visitor(new ToStringVisitor(&ss));
+
+				ss << "Own parameter " << this->get_name() << "::" << tag.name() << "=";


I would actually like to print the differences here (also for vectors which is currently shown as ...), as that was useful in unit-testing in the past

It could be a custom visitor. Or we can add another interface 'Reducer'.

lisitsyn · 2018-01-05T21:11:01Z

src/shogun/base/AnyParameter.h

@@ -88,6 +88,16 @@ namespace shogun
 			return m_properties;
 		}

+		inline bool operator==(const AnyParameter& other)
+		{
+			return m_value == other.get_value();


Probably ok. Maybe needs a comment that it doesn't consider properties.

lisitsyn · 2018-01-05T21:11:23Z

src/shogun/base/SGObject.cpp

@@ -28,6 +28,7 @@

 #include <stdlib.h>
 #include <stdio.h>
+#include <typeinfo>


Was it missed?

lisitsyn · 2018-01-05T21:12:32Z

src/shogun/base/SGObject.cpp

+	}
+
+	/* Assumption: objects of same type have same set of tags. */
+	for (auto it=self->map.cbegin(); it!=self->map.cend(); it++)


Why not auto it : self->map?

lisitsyn · 2018-01-05T21:13:11Z

src/shogun/base/SGObject.cpp

+				std::stringstream ss;
+				std::unique_ptr<AnyVisitor> visitor(new ToStringVisitor(&ss));
+
+				ss << "Own parameter " << this->get_name() << "::" << tag.name() << "=";


It could be a custom visitor. Or we can add another interface 'Reducer'.

lisitsyn · 2018-01-05T21:13:28Z

src/shogun/lib/any.h

@@ -153,7 +153,12 @@ namespace shogun
 		auto compare_impl(maybe_most_important, T* lhs, T* rhs)
 		    -> decltype(lhs->equals(rhs))
 		{
-			return lhs->equals(rhs);
+			if (lhs && rhs)


Yeah that's a good catch.

lisitsyn · 2018-01-06T13:00:46Z

You might want to fix the codestyle ;)

git clang-format-3.8 --commit 6f1fd162892988e212b5ddfc6de15db3a8bf1026 --binary /usr/bin/clang-format-3.8

karlnapf · 2018-01-07T18:02:11Z

tests/unit/base/SGObject_unittest.cc

-}
-
-TEST(SGObject,equals_DynamicObjectArray_equal)
+TEST(SGObject,DISABLED_equals_DynamicObjectArray_equal)


@lisitsyn I had to disable this until we fix the registration of that partly used array parameter

Via operator== of SGSparseVectorEntry. Test for all types.

move logic to CMath::fequals where the global over-ride is respected

karlnapf · 2018-01-08T17:54:16Z

@lisitsyn ok it was green now apart form style, which I fixed. Let's wait for travis to be sure, but you can review now. And potentially merge

karlnapf · 2018-01-08T17:56:03Z

src/shogun/base/init.cpp

@@ -147,6 +154,26 @@ namespace shogun
 		return sg_io;
 	}

+	float64_t get_global_fequals_epsilon()
+	{
+		return sg_fequals_epsilon;


@vigsterkr this is fucked up, but the way it was before was even more so. We should just get rid of all of this accuracy business and rather fix the serialization, which should not be lossy (in my eyes)

karlnapf · 2018-01-08T17:57:01Z

src/shogun/mathematics/Math.h


 				// Handle this separately since NAN is unordered
 				if (CMath::is_nan((float64_t)a) && CMath::is_nan((float64_t)b))
 					return true;

 				// Required for JSON Serialization Tests
-				if (tolerant)
-					return CMath::fequals_abs<T>(a, b, eps);
+			    if (get_global_fequals_tolerant())


@vigsterkr here the beauty is

karlnapf · 2018-01-08T17:57:18Z

src/shogun/mathematics/Math.h

+		    {
+			    // global fequals epsilon might override passed one
+			    // hack for lossy serialization formats
+			    float64_t eps = std::max(eps_, get_global_fequals_epsilon());


here it comes ...

karlnapf · 2018-01-08T17:59:10Z

tests/unit/base/SGObject_unittest.cc


 	SG_UNREF(array1);
 	SG_UNREF(array2);
 }

-TEST(SGObject,equals_DynamicObjectArray_equal_after_resize)
+TEST(SGObject, DISABLED_equals_DynamicObjectArray_equal_after_resize)


@lisitsyn @vigsterkr I disabled this for now. With this patch, DynamicObjectArray is not serializable anymore. We need to fix the registration of the underlying array (not all of it, only used parts)

lisitsyn · 2018-01-08T18:37:00Z

Ok this is deffs improvement even though accuracy business is meh :)

karlnapf · 2018-01-08T22:26:14Z

Ok baby, lets merge it, just fixed the style issues....

karlnapf · 2018-01-08T22:28:45Z

I take that back, windows is not happy, https://ci.appveyor.com/project/vigsterkr/shogun/build/1668#L16427

karlnapf · 2018-01-09T18:56:36Z

One unit test was actually broken (and under linux only passed accidentally with an invalid read memory error). Should be fixed now.

karlnapf · 2018-01-09T23:49:57Z

windows times out and travis is angry about style (even though I ran the checker, I think it is confused).
I think that means merge ... Objections?

karlnapf · 2018-01-09T23:51:01Z

I'll just do it and fix tomorrow if there are problems

karlnapf commented Jan 5, 2018

View reviewed changes

karlnapf requested a review from lisitsyn January 5, 2018 18:45

karlnapf commented Jan 5, 2018

View reviewed changes

lisitsyn approved these changes Jan 5, 2018

View reviewed changes

karlnapf force-pushed the feature/equals_with_tags branch from 44421a7 to a61faaa Compare January 7, 2018 17:53

karlnapf commented Jan 7, 2018

View reviewed changes

karlnapf force-pushed the feature/equals_with_tags branch 3 times, most recently from 9c78e6f to 60531cb Compare January 8, 2018 16:06

karlnapf mentioned this pull request Jan 8, 2018

EASY: create factory methods to create LinearMachine instances by their name #4044

Closed

karlnapf force-pushed the feature/equals_with_tags branch from 2fd32e4 to 4203d62 Compare January 8, 2018 16:58

karlnapf added 13 commits January 8, 2018 17:52

fix any equals if one argument is nullptr

2f975ff

fix equals method when both matrices are empty

eb1c0db

remove pointless debug messages

1e7d2a8

test equals in symmetric way

1dff7cf

add debug messages for comparison of any

8dec3e0

Add equals method to SGSparseVector

95a0bae

Via operator== of SGSparseVectorEntry. Test for all types.

Implement tag-based equals implementation

8867608

fix incorrect assertion

6dbc02c

remove old equals code

2eb332e

add fequals based equals for floats

b7c47dc

replace "accuracy" parameters in CSGObject::equals with global variable

6086072

move logic to CMath::fequals where the global over-ride is respected

add element-wise float comparison over fequals

47b9538

fix unit tests regarding ::equals API without accuracy

64c4f7d

karlnapf force-pushed the feature/equals_with_tags branch from 4203d62 to 48e0b1b Compare January 8, 2018 17:53

karlnapf commented Jan 8, 2018

View reviewed changes

karlnapf force-pushed the feature/equals_with_tags branch from 48e0b1b to 17523b6 Compare January 8, 2018 22:24

karlnapf added 3 commits January 9, 2018 18:53

fix equals unit test with different dimensions

c2e7801

get rid of unused variable warning

b681f6c

apply autoformater

a53ce00

karlnapf force-pushed the feature/equals_with_tags branch from 17523b6 to a53ce00 Compare January 9, 2018 18:55

karlnapf merged commit e4a19a2 into shogun-toolbox:develop Jan 9, 2018

karlnapf deleted the feature/equals_with_tags branch January 9, 2018 23:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/equals with tags #4069

Feature/equals with tags #4069

karlnapf commented Jan 5, 2018 •

edited

karlnapf Jan 5, 2018

lisitsyn Jan 5, 2018

karlnapf Jan 5, 2018

lisitsyn Jan 5, 2018

karlnapf Jan 5, 2018

lisitsyn Jan 5, 2018

karlnapf Jan 5, 2018

karlnapf Jan 5, 2018

karlnapf Jan 5, 2018

karlnapf Jan 5, 2018

lisitsyn Jan 5, 2018

lisitsyn Jan 5, 2018

lisitsyn Jan 5, 2018

lisitsyn Jan 5, 2018

lisitsyn Jan 5, 2018

lisitsyn Jan 5, 2018

lisitsyn commented Jan 6, 2018

karlnapf Jan 7, 2018

karlnapf commented Jan 8, 2018

karlnapf Jan 8, 2018

karlnapf Jan 8, 2018

karlnapf Jan 8, 2018

karlnapf Jan 8, 2018

lisitsyn commented Jan 8, 2018

karlnapf commented Jan 8, 2018

karlnapf commented Jan 8, 2018

karlnapf commented Jan 9, 2018

karlnapf commented Jan 9, 2018

karlnapf commented Jan 9, 2018

Feature/equals with tags #4069

Feature/equals with tags #4069

Conversation

karlnapf commented Jan 5, 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

lisitsyn commented Jan 6, 2018

Choose a reason for hiding this comment

karlnapf commented Jan 8, 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

lisitsyn commented Jan 8, 2018

karlnapf commented Jan 8, 2018

karlnapf commented Jan 8, 2018

karlnapf commented Jan 9, 2018

karlnapf commented Jan 9, 2018

karlnapf commented Jan 9, 2018

karlnapf commented Jan 5, 2018 •

edited