Refactored linalg operations tests ( addresses #4383) #4405

gf712 · 2018-10-15T15:12:27Z

Refactored all the linalg Eigen3 operations in Eigen3_operations_unittest.cc as suggested in #4383.

In addtion to typed testing, changes include:

Decreased tolerance for float type tests to work with float32_t, float64_t and floatmax_t
Different values in tests where the same code has to work with both int and float types

I did not add complex128_t, bool or char tests at this point.

I just need help writing a test for SGMatrix_squared_error as this potentially requires changing CMath::pow (mentioned in #4383). At this point I left the original test.

I know it's quite a large number of lines changed in one PR, but this is mainly a code refactor and the tests are almost identical.

All tests run fine locally!

Refactored all the opeations tests. * Decreased tolerance for float type tests to work with `float32_t`, `float64_t` and `floatmax_t` * Changed tests where `int` types are required to work with both `int` and `float` * Did not add `complex128_t`, `bool` or `char` tests

I added the SE test whilst there is no fix for the typed test for this case (see my PR description)

karlnapf

Hi

Wow this is a big patch! We really appreciate your efforts! I put a few inline comments for a first iteration.

A few questions:
-Did you run all the tests locally?
-They pass?
-No memory errors?

Our CI currently has issues and therefore didnt execute them (we will fix that soon) this is why I ask

karlnapf · 2018-10-17T10:15:04Z

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

+// TODO: make global definitions
+// Definition of the 4 groups of Shogun types (shogun/mathematics/linalg/LinalgBackendBase.h)
+// TODO: add bool, chars and complex128_t types
+typedef ::testing::Types<int8_t, int16_t, uint16_t, int32_t, uint32_t, int64_t, uint64_t, float32_t, float64_t, floatmax_t> AllTypes;


karlnapf · 2018-10-17T10:15:30Z

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

-	}
+TYPED_TEST_CASE(LinalgBackendEigenAllTypesTest, AllTypes);
+TYPED_TEST_CASE(LinalgBackendEigenNonComplexTypesTest, NonComplexTypes);
+TYPED_TEST_CASE(LinalgBackendEigenRealTypesTest, RealTypes);


@vigsterkr what's your opinion in making those things globally visible so that we wont have to re-declare them?

karlnapf · 2018-10-17T10:15:43Z

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

+
+
+template <typename ST>
+void check_SGVector_add() {


What is the motivation to have the "check_" prefix here?

karlnapf · 2018-10-17T10:17:18Z

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

-	}
+    for (index_t i = 0; i < 9; ++i)
+    {
+    A[i] = i;


there are some minor formatting issues here.
You an actually automatically format this properly... see the output from the first travis job here:
https://travis-ci.org/shogun-toolbox/shogun/jobs/441713579#L723

This would also make the diff of this PR much smaller, as there are fewer whitespace changes

karlnapf · 2018-10-17T10:19:57Z

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

-	SGMatrix<float64_t> A(nrows, ncols);
-	SGMatrix<float64_t> B(nrows, ncols);
-	SGMatrix<float64_t> C(nrows, ncols);
+    const ST alpha = 1;


uh I didn't reallise that some tests actually operated with floating point number constants and we now have to change all those numbers. Quite some work! Thanks for going into this!

karlnapf · 2018-10-17T10:21:52Z

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

 {
 	const index_t size=2;
-	SGMatrix<float64_t> m(size, size);
+	SGMatrix<ST> m(size, size);
+	typedef Matrix<ST, Dynamic, Dynamic> Mxx; // need to adapt the Eigen::Matrix to ST type


I think we might have something similar in SGVector.h

karlnapf · 2018-10-17T10:22:33Z

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

 	EXPECT_NEAR((map_A-map_L*map_L.transpose()).norm(),
-		0.0, 1E-15);
+		0.0, 1E-6); // need to change the treshold to work with floats


as an idea, we could use a templated function get_epsilon<T> that looks up the appropriate epsilons?

karlnapf · 2018-10-17T10:23:00Z

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

+    A2(1, 1) += b[1] * b[1];
+
+    cholesky_rank_update(U, b, alpha, false);
+    EXPECT_NEAR((A2_eig - U_eig.transpose() * U_eig).norm(), 0.0, 1e-5);  // need to change the treshold to work with floats


minor style: we do comments in a line above usually (+ a whitespace line above that)

karlnapf · 2018-10-17T10:27:00Z

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


-	SGMatrix<float64_t> A(data_A, n, n, fa


What is the motivation for putting these one liners in here rather than the test code directly?
How does it look when a test fails? Is it easy to exactly identify where the program is coming from? Sometimes with googltest, it is hard to figure out which case the tester is exactly in

Or in other words: Why not put the test code in there straight away and use TypeParam instead of ST?

Hi, it seems like my previous answer disappeared. There was no specific motivation for this. I just used other code in shogun as a template. The way it was written here is the same as in shogun/tests/unit/classifier/LDA_unittest.cc. The error traceback works fine when the test fails.

karlnapf · 2018-10-17T10:28:33Z

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


-	SGMatrix<float64_t> A(data_A, n, n, fa


not sure the method is even defined for int types?
Generally, we try to avoid TODOs in the codebase (they never get addressed ;)

does a call to this method with int type even compile?

Yes, this compiles, but it might not make much sense...

shogun/src/shogun/mathematics/linalg/LinalgBackendBase.h

Lines 255 to 262 in f6efd89

#define BACKEND_GENERIC_RANK_UPDATE(Type, Container) \

virtual void rank_update( \

Container<Type>& A, const SGVector<Type>& b, Type alpha) const \

{ \

SG_SNOTIMPLEMENTED; \

}

DEFINE_FOR_ALL_PTYPE(BACKEND_GENERIC_RANK_UPDATE, SGMatrix)

#undef BACKEND_GENERIC_RANK_UPDATE

well as long as the eigen3 call goes through it makes sense to offer it :)
But I am surprised ...

gf712 · 2018-10-17T14:20:49Z

Did you run all the tests locally?
Yes, all tests run fine locally, but it would be better to first implementing a default tolerance level for each type to avoid any issues!
No memory errors?
I had no memory issues when I run with valgrind. But when running with valgrind one of the tests fails (it doesn't fail when just running unittest). In the case ofLinalgBackendEigenNonIntegerTypesTest/2.SGMatrix_pinv_psd, where TypeParam = long double pinv returns nan for all matrix elements.

* get_epsilon is called inside each test case to determine the tolerance * tested these values with my own machine * solver tests will have a lower threshold, as these seem to change quite a bit depending on machine and lapack/blas installations * if typed testing is rolled out for all unit tests should move this to its own header file

* solvers have a fixed threshold * also fixed indentation

karlnapf · 2018-10-22T08:31:02Z

Hi Gil,
I am quite busy today, but I will try to get back to this PR tomorrow latest.

The error when running with valgrind seems strange, this usually happens when there is an uninitialized read somewhere in the code (something which valgrind shows). For the pinv, it might be worth checking how it is implemented and then dig in the code and eigen3 docs to see whether the way it is done works with long (eigen might bail actually). If it is unfixable, we will have to remove the definition for long

karlnapf · 2018-10-22T09:01:37Z

Hi Gil,
I am quite busy today, but I will try to get back to this PR tomorrow latest.

The error when running with valgrind seems strange, this usually happens when there is an uninitialized read somewhere in the code (something which valgrind shows). For the pinv, it might be worth checking how it is implemented and then dig in the code and eigen3 docs to see whether the way it is done works with long (eigen might bail actually). If it is unfixable, we will have to remove the definition for long

gf712 · 2018-10-23T11:47:09Z

Hi, I will have a better look at it today. Github wasn't working properly so I didn't really want to push any code.
It's just odd that the pinv test runs fine with long double without using valgrind..!

gf712 · 2018-10-23T13:40:48Z

It seems like after changing some values of the matrix tested by SGMatrix_pinv_psd gives the correct answer. However, I'm not sure why this is the case.
Printing out the individual components of the SVD decomposition shows that for some reason Eigen's JacobiSVD returns nan values, but only when using valgrind with long double, when there is a matrix with this format:

[[a b c]
 [b d b]
 [e b f]]

By changing one of any b values seems to give a valid answer..

karlnapf · 2018-10-26T10:15:30Z

It might be worth filing this as a bug report with the eigen3 guys, they are quick in fixing bugs

karlnapf

made a few more relatively minor comments

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

karlnapf · 2018-10-26T10:19:30Z

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


-	SGMatrix<float64_t> A(data_A, n, n, fa


does a call to this method with int type even compile?

karlnapf · 2018-11-07T09:58:41Z

What's missing for this to be finished?

gf712 · 2018-11-07T13:49:34Z

I think all that is missing is the addition of bool, chars and complex type tests. I am just not sure how those test would look like..? For example you could compile a test with dot product of bool and char types but how would that test look like/be interpreted?

karlnapf · 2018-11-07T14:54:33Z

We should follow c++ is_arithmetic. Then remove the types that eigen3 cannot deal with, or those where it just doesn't make any sense

karlnapf

This looks good to me now.

Only thing that would be cool to sort out otherwise is the definition of the testing types.
But maybe we can do that in another patch, this is big enough already.

@gf712 are you happy to get this merged ?

gf712 · 2018-11-12T13:37:02Z

Yes, I agree, in another patch could move the definitions to a header file with definitions that can be accessed by all tests?
The only thing missing here would be the complex type... But might have to write special test cases for these no?

karlnapf · 2018-11-12T17:33:44Z

Yes, I agree, in another patch could move the definitions to a header file with definitions that can be accessed by all tests?

yes

The only thing missing here would be the complex type... But might have to write special test cases for these no?
You might have to for some. For now we can leave that and maybe investigate it later (e.g. when someone needs it.

Ok then, I will wait for a few more days for others to comment and then merge this

…hogun-toolbox#4405) * Refactored linalg operations tests * Changed tests where `int` types are required to work with both `int` and `float` * Did not add `complex128_t`, `bool` or `char` tests * added original SE test * solver tests will have a lower threshold, as these seem to change quite a bit depending on machine and lapack/blas installations * automatic tolerance detection

Gil added 2 commits October 15, 2018 15:48

added original SE test

00dcc2f

I added the SE test whilst there is no fix for the typed test for this case (see my PR description)

karlnapf reviewed Oct 17, 2018

View reviewed changes

Gil added 3 commits October 19, 2018 11:10

added get_epsilon to tests

b1fb379

* solvers have a fixed threshold * also fixed indentation

template formatting

02298ad

karlnapf requested changes Oct 26, 2018

View reviewed changes

Gil added 2 commits November 8, 2018 11:00

automatic tolerance detection

d94653f

moved code to TYPED_TEST

64668b5

karlnapf approved these changes Nov 12, 2018

View reviewed changes

karlnapf merged commit 6e42c52 into shogun-toolbox:develop Nov 15, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactored linalg operations tests ( addresses #4383) #4405

Refactored linalg operations tests ( addresses #4383) #4405

gf712 commented Oct 15, 2018

karlnapf left a comment

karlnapf Oct 17, 2018

karlnapf Oct 17, 2018

karlnapf Oct 17, 2018

karlnapf Oct 17, 2018

karlnapf Oct 17, 2018

karlnapf Oct 17, 2018

karlnapf Oct 17, 2018

karlnapf Oct 17, 2018

karlnapf Oct 17, 2018

karlnapf Oct 17, 2018

karlnapf Oct 17, 2018

gf712 Nov 1, 2018

karlnapf Oct 17, 2018

karlnapf Oct 26, 2018

gf712 Nov 1, 2018 •

edited

karlnapf Nov 5, 2018

gf712 commented Oct 17, 2018

karlnapf commented Oct 22, 2018

karlnapf commented Oct 22, 2018

gf712 commented Oct 23, 2018

gf712 commented Oct 23, 2018

karlnapf commented Oct 26, 2018

karlnapf left a comment

karlnapf Oct 26, 2018

karlnapf commented Nov 7, 2018

gf712 commented Nov 7, 2018

karlnapf commented Nov 7, 2018

karlnapf left a comment

gf712 commented Nov 12, 2018

karlnapf commented Nov 12, 2018 •

edited

	#define BACKEND_GENERIC_RANK_UPDATE(Type, Container) \
	virtual void rank_update( \
	Container<Type>& A, const SGVector<Type>& b, Type alpha) const \
	{ \
	SG_SNOTIMPLEMENTED; \
	}
	DEFINE_FOR_ALL_PTYPE(BACKEND_GENERIC_RANK_UPDATE, SGMatrix)
	#undef BACKEND_GENERIC_RANK_UPDATE

Refactored linalg operations tests ( addresses #4383) #4405

Refactored linalg operations tests ( addresses #4383) #4405

Conversation

gf712 commented Oct 15, 2018

karlnapf left a 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

gf712 Nov 1, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gf712 commented Oct 17, 2018

karlnapf commented Oct 22, 2018

karlnapf commented Oct 22, 2018

gf712 commented Oct 23, 2018

gf712 commented Oct 23, 2018

karlnapf commented Oct 26, 2018

karlnapf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karlnapf commented Nov 7, 2018

gf712 commented Nov 7, 2018

karlnapf commented Nov 7, 2018

karlnapf left a comment

Choose a reason for hiding this comment

gf712 commented Nov 12, 2018

karlnapf commented Nov 12, 2018 • edited

gf712 Nov 1, 2018 •

edited

karlnapf commented Nov 12, 2018 •

edited