[WIP] RFFGaussPreproc #5010

jonpsy · 2020-04-19T11:49:06Z

Addressing #5009

src/shogun/preprocessor/RandomFourierGaussPreproc.cpp

src/shogun/preprocessor/RandomFourierGaussPreproc.h

src/shogun/preprocessor/RandomFourierGaussPreproc.cpp

jonpsy · 2020-04-19T15:08:24Z

Proposal: Modify set_randomcoefficients
Reason: It takes in lots of parameters and set internal members value to that.
Rather
a) Take const ref and extract values from it
b) set each internal member value manually, there aren't many.

Also, rename to extract_randomcoefficients because it takes values from the arguments and set internal values from there and not vice versa.

EDIT: Implemented it, see the updated PR. Yet to remove is_updated

src/shogun/preprocessor/RandomFourierGaussPreproc.cpp

karlnapf · 2020-04-19T19:51:49Z

src/shogun/preprocessor/RandomFourierGaussPreproc.cpp

-	    randomcoeff_additive, randomcoeff_additive + cur_dim_feature_space, 0.0,
-	    2 * pi, m_prng);
+	    m_randomoffset, m_randomoffset + m_dim_feature_space, 0.0,
+	    2 * M_PI, m_prng);


where is M_PI coming from? Use the one in std::

M_PI is from the cmath include, luckily it's already included it so it all blends in perfectly. We don't have std::pi. We can, however, use const double pi = std::acos(-1); but that's a waste of LoC and even it's precision is lesser than M_PI:

@karlnapf C++ library doesn't have pi definition, it relies on cmath. The best we could do is static constexpr double PI = 3.1415... or have an overloaded struct shogun::constants<float>::pi, shogun::constants<double>::pi and so on. @nanusai avoid calculating constants inside an algorithm, make such things static and constexpr if possible

Also calculating why calculate pi if it is a known constant?

There seems to be a misunderstanding, I already am using M_PI. I was just explaining why I was using it.Anyway I guess we all agree on M_PI ;D.

EDIT: Good news, we've already Math::PI inside our shogun::Math

const float64_t Math::PI=M_PI;

we want to drop that class eventually, but I guess you can use that for now.

@gf712 shall we put the constexpr in linalg maybe?

src/shogun/preprocessor/RandomFourierGaussPreproc.cpp

karlnapf · 2020-06-08T13:50:14Z

src/shogun/mathematics/linalg/LinalgNamespace.h

@@ -1325,6 +1325,39 @@ namespace shogun
 			return result;
 		}

+		/** Performs the operation B = cos(A)


noooooo
You would cherry pick your other commit instead of force pushing this into here. This just wastes CI cycles.
git is a great tool and it can avoid exactly those situations

now (with the other PR merged), you need to force push here again. If you had cherry picked the commit, this wouldnt have been necessary (git would have worked it out for us)
next time :)

this still needs to be removed

karlnapf · 2020-06-08T13:50:48Z

src/shogun/preprocessor/RandomFourierGaussPreproc.cpp


-	SGMatrix<float64_t> res(cur_dim_feature_space, num_vectors);
+	linalg::matrix_prod(m_basis,matrix,projection);


karlnapf · 2020-06-08T13:54:22Z

src/shogun/preprocessor/RandomFourierGaussPreproc.cpp

-	SGVector<float64_t> res(cur_dim_feature_space);
+	assert_fitted();
+	linalg::matrix_prod(m_basis, vector, projection);
+	linalg::add(projection, m_offset, projection);


Ok one more thing. Sorry!
But this code simply replicates the matrix code. Why have it twice? It is pretty much copy pasted ....

You can just call the matrix code from here. All you need to do is to make a matrix from the input vector
return apply_to_feature_matrix(SGMatrix<...>(vector)).get_column_vector(0) (double check the details ... )

you would have to make the column vector persistent somehow as well

src/shogun/preprocessor/RandomFourierGaussPreproc.cpp

gf712 · 2020-06-09T06:12:19Z

src/shogun/preprocessor/RandomFourierGaussPreproc.cpp

-			    cur_dim_input_space, false);
-			res(od, vec) =
-			    val * cos(randomcoeff_additive[od] + linalg::dot(a, b));
+			auto width = std::exp(m_log_width * 2.0) * 2.0;


Suggested change

auto width = std::exp(m_log_width * 2.0) * 2.0;

const auto width = std::exp(m_log_width * 2.0) * 2.0;

I am not sure the compiler will optimise this, but you might be better off computing this outside the loop?

The compiler should optimize variable declaration inside the loop. This is also a good practice I've heard because this limits the scope of variable.

are you sure? because m_log_width could be changed in another thread, so how can the compiler assume this will always be the same?

you do have a point. i guess putting the width inside loop doesn't make too much sense either. Okay then

btw I think the optimisations it could do here is reuse the memory allocation on the stack and then the CPU itself can see that you are recomputing the same all the time and put the result in the hot path. If m_log_width was const, then I think the compiler would compute the value only once.

making data members as const would render the setter useless, wouldn't it? I've put width outside the loop, and I think that's for better.

i am just telling you when there could be optimisations. You could imagine copying it to a const variable to test it out. But yes, you should put it outside the loop to be sure.

I see, thanks. No more loops btw :)

gf712 · 2020-06-09T06:16:29Z

src/shogun/preprocessor/RandomFourierGaussPreproc.cpp


+	for (auto row : range(m_dim_output))
+		m_offset[row] = uniform(m_prng);


this is exactly the use case for something like std::transform, which will give you better performance

gf712 · 2020-06-09T06:17:08Z

src/shogun/preprocessor/RandomFourierGaussPreproc.cpp

+	require(num_features > 0, "Input space dimension must be greater than zero");
+
+	init_basis(num_features);
+	m_fitted = true;
 }

 SGVector<float64_t> RandomFourierGaussPreproc::apply_to_feature_vector(SGVector<float64_t> vector)


since you are refactoring this can you change the signature to const SGVector<float64_t>& please?

this signature is inherited, would have to change a lot of files for it to work. thoughts?

hmm I guess let it be for another PR then :)

yes, there will be a follow up refactoring the whole class structure.

gf712 · 2020-06-09T06:17:27Z

src/shogun/preprocessor/RandomFourierGaussPreproc.cpp

 }

-SGMatrix<float64_t>
-RandomFourierGaussPreproc::apply_to_matrix(SGMatrix<float64_t> matrix)
+SGMatrix<float64_t> RandomFourierGaussPreproc::apply_to_matrix(SGMatrix<float64_t> matrix)


const SGMatrix<float64_t>&

marked as resolved, but not resolved.
@jonpsy PLEASE dont mark anything as resolved, especially not the ones that are not changed. If you want to do it, do it AFTER you have pushed changes .... but better yet, just leave those comments in, they give us a good feeling of what happened and people can hide them if they want

actually, this is for another PR as well.... so next time, just state that rather than hiding it ...

not "hiding" per se, we've had VERY similar discussion above if you recall. So to avoid repeating same question..i did that.

sorry i dont mean "hiding" in the sense that you are hiding stuff from us, but rather the label of the button :)
Again, please just don't click it, it causes confusion and takes away time from other stuff.
In a situation like this, just say "will do in next PR when we refactor the interfaces of the base classes". And then it is there and everyone immediately knows what is going on, and if I dont want to see it (or you) everyone can just hide it personally

gf712 · 2020-06-09T07:55:38Z

src/shogun/preprocessor/RandomFourierGaussPreproc.cpp

+	    [this, &normal_dist, &width]() {
+		    auto coeff = std::sqrt(2.0 / width) * normal_dist(m_prng);
+		    return coeff;
+	    });


this looks much nicer in my opinion! And it should reduce cache misses. Minor thing, why don't you just return the right hand side of coeff?

btw why not store the full multiplier outside of the loop, including std::sqrt(2.0 / width) ?

yes, that could be done. In this situation I would expect a good compiler to just compute it once, because width is const. But to be sure can just compute this outside the lambda

karlnapf · 2020-06-09T09:49:50Z

src/shogun/preprocessor/RandomFourierGaussPreproc.cpp

-
-	return res;
+	return static_cast<SGVector<float64_t>>(
+	    apply_to_matrix(static_cast<SGMatrix<float64_t>>(vector)));


does this actually work and is safe?
@gf712 I didnt know we can just cast these types back and forth ....

especially the cast from SGMatrix->SGVector ... what if there are more than one column in the matrix (it wont be here) is this checked somewhere?

I thought doing this via a constructor would be better/safer (as said in irc @jonpsy )

this will just call

shogun/src/shogun/lib/SGMatrix.h

Line 116 in ab6a696

SGMatrix(SGVector<T> vec);

and

shogun/src/shogun/lib/SGVector.h

Line 72 in ab6a696

SGVector(SGMatrix<T> matrix);

https://github.com/shogun-toolbox/shogun/blob/develop/src/shogun/lib/SGMatrix.h#L116

https://github.com/shogun-toolbox/shogun/blob/develop/src/shogun/lib/SGVector.h#L72

I just learned that static_cast in fact calls the ctor. Nevermind then :)

sorry my inet lagged and didnt see your reply. Cool!

karlnapf · 2020-06-09T10:06:29Z

the CI tests still fail?

gf712 · 2020-06-09T10:10:07Z

the CI tests still fail?

yes, the data file has to be updated, because @jonpsy switched the random number matrix generation from row to column major (to reduce cache misses). He just has to amend the data commit right?

karlnapf · 2020-06-09T10:16:54Z

ah of course, yes!
BTW imo there really should be a method that fills random arrays in this way, that takes care of the row/column major, to prevent people shooting into their feet ...

gf712 · 2020-06-09T12:07:35Z

BTW imo there really should be a method that fills random arrays in this way, that takes care of the row/column major, to prevent people shooting into their feet ...

But we would just be wrapping around std::generate? I mean we could potentially extend fill_array with an overload for fill_array(matrix, generator)/fill_array(matrix.begin(), matrix.end(), generator)

karlnapf · 2020-06-09T12:45:03Z

maybe you are right, just thinking about how many times I asked someone to use std::generate or std::transform rather than a loop, and how many times people don't respect linearity in memory by for example iterating row-wise....

karlnapf · 2020-06-09T17:27:22Z

src/shogun/preprocessor/RandomFourierGaussPreproc.cpp

-
-	return false;
+RandomFourierGaussPreproc::RandomFourierGaussPreproc()
+: m_log_width(1.0), m_dim_output(100)


the effective kernel width should be 1
Also, could you set these values at the declaration

karlnapf · 2020-06-09T17:30:05Z

src/shogun/preprocessor/RandomFourierGaussPreproc.cpp

+	NormalDistribution<float64_t> normal_dist;
+	auto sampled_kernel = SGMatrix<float64_t>(m_dim_output, dim_input_space);
+	const auto width = std::exp(m_log_width * 2.0) * 2.0;
+	const auto factor = std::sqrt(2.0 / width);


could name this std_dev, but it doesnt really matter

yep, as it is the standard deviation of the gaussian you sample from below

i.e. factor -> std_dev ... but only if your nerves allow ;)

karlnapf · 2020-06-09T17:52:26Z

src/shogun/preprocessor/RandomFourierGaussPreproc.cpp

+{
+	NormalDistribution<float64_t> normal_dist;
+	auto sampled_kernel = SGMatrix<float64_t>(m_dim_output, dim_input_space);
+	const auto width = std::exp(m_log_width * 2.0) * 2.0;


gna I oversaw this one earlier. You should use get_width here to avoid double code

(try to never copy paste code)

karlnapf · 2020-06-09T20:17:12Z

@gf712 if you don't have any further concerns, I will merge this beast ...

gf712 · 2020-06-10T07:18:14Z

@gf712 if you don't have any further concerns, I will merge this beast ...

sounds good, I think this is ready to be merged! Great work @jonpsy :)

* add RFGPbenchmark * add fit benchmark

karlnapf reviewed Apr 19, 2020

View reviewed changes