Linalg Refactor - CPU dot #3230

OXPHOS · 2016-05-30T16:11:03Z

I'll separate classes into different .h and .cpp later
Not much work in GPU part yet.
Works for Eigen backend, dot(a, b)

@lambday @vigsterkr @karlnapf

karlnapf · 2016-05-30T17:13:42Z

Cool!
I guess next step would be to test a GPU dot product, and then two cases:

GPU lib is available (and dot is computed on GPU after a transfer)
- GPU lib is not available (fall back to CPU)

@lambday should also comment

lambday · 2016-05-30T22:32:56Z

src/shogun/mathematics/linalg/linalgRefactor.h

+struct CPU_Vector : public BaseVector<T>
+{
+    //<SGVector<T>>* CPUptr;
+    SGVector<T> vec;


can we just hold a reference to the SGVector instead instead? We won't be passing this CPUVector around via any API (apart from linalg API which is internal), so an extra refcount is not really needed I think.

lambday · 2016-05-30T23:06:24Z

Alright this looks okay! Needs some more polishing.

Apart from what @karlnapf already mentioned,

Split the files, make sure it works after splitting
Use the pimpl pattern to remove the dependency from ViennaCL in the headers
Draft a small POC of how the Factory class should work. This class would be responsible for creating the instances of CPUVectors or GPUVectors. Things to keep in mind - we want the construction to be as lightweight as possible, i.e. avoid reference-counting if it is not really needed, etc
Change the name LinalgRefactor to something that's gonna stay. We need a global instance of that class. The methods should be state-less and thread-safe. Make sure that they are.
Benchmark MUST. Write a small example in which (a) we do the dot using eigen3 directly and (b) we use this linalg dot (you use benchmarking libraries google-benchmark/hayai etc) - take small to large vectors, run 1000 times, check how much we're paying for all this flexibility.

OXPHOS · 2016-06-01T05:24:57Z

@vigsterkr @lambday There're many ways to put ifdef in .cpp for dot(). I currently didn't use opaque pointer because I can only think of Class - Member Function - Class, kinda complicated..?

But on a second thought I don't think my current implementation is interchangeable..

lambday · 2016-06-01T10:08:03Z

@OXPHOS please put one class per file. So there will be

BaseVector.h
CPUVector.h
CPUVector.cpp
GPUVector.h
GPUVector.cpp
Linalg.h
Linalg.cpp
CPUBackend.h
CPUBackend.cpp
GPUBackend.h
GPUBackend.cpp

When you split things like that, it would be easier to refactor things with opaque ptrs.

OXPHOS · 2016-06-01T16:22:22Z

@lambday please check. thx!

lambday · 2016-06-01T16:33:47Z

src/shogun/mathematics/linalgrefactor/BaseVector.h

+#include <shogun/lib/config.h>
+#include <shogun/lib/SGVector.h>
+
+#ifndef _BASEVECTOR_H__


get rid of the leading _. those are reserved for system headers. We should just use BASEVECTOR_H__

OXPHOS · 2016-06-02T04:59:02Z

@lambday dunno why it's failing some tests..also this doesn't seem to be thread safe?

lambday · 2016-06-02T11:53:11Z

src/shogun/mathematics/linalgrefactor/GPUBackend.cpp

+    */
+    T compute(GPU_Vector<T> a, GPU_Vector<T> b)
+    {
+       return viennacl::linalg::inner_prod(*(a.GPUptr), *(b.GPUptr));


What is this GPUptr? It's not declared or defined in GPU_Vector class.

Shouldn't it be working with the VCLMemoryArray shared ptr thing that it has?

lambday · 2016-06-07T23:08:22Z

src/shogun/mathematics/linalgrefactor/CPUVector.h

+
+    CPUVector(const CPUVector<T> &vector);
+
+    bool onGPU() { return false; }


inline bool onGPU() const { return false; }

OXPHOS · 2016-06-08T04:25:22Z

I think Heiko @karlnapf showed me a header different from
https://github.com/shogun-toolbox/shogun/blob/develop/src/shogun/statistics/HypothesisTest.h
last time but I cannot find it for now.. so I didn't add the header yet.
For unit-test case: GPU convert (i.e. test whether SGVector can be convert to GPU), it requires element access [] which needs more work

lambday · 2016-06-08T11:23:56Z

Hi @OXPHOS

Doesn't matter. Use whichever you want. But license has to be there.
Can you bypass requiring the operator[] by casting it to viennacl vector type and using it's access operator?

OXPHOS · 2016-06-08T16:16:35Z

@lambday both done.

lambday · 2016-06-08T17:38:55Z

src/shogun/base/init.cpp

@@ -37,6 +38,7 @@ namespace shogun
 	Version* sg_version=NULL;
 	CMath* sg_math=NULL;
 	CRandom* sg_rand=NULL;
+	std::unique_ptr<SGLinalg> sg_linalg(nullptr);


can you please try using shogun's Unique here? if that works then we won't need c++11 guard around all these

lambday · 2016-06-09T10:51:30Z

src/shogun/base/init.h

+ *
+ * @param linalg linalg object to use
+ */
+void set_global_linlg(SGLinalg* linalg);


Is it defined somewhere? I see definition of get_global_linalg() in the cpp but not this one.

Also, maybe this is not needed. Just remove this for now :)

lambday · 2016-06-09T11:06:25Z

@OXPHOS really nice job with this :) Let me merge this for now. Travis fails seem unrelated, Plus this is a feature branch.

Check my minor comments. Maybe you can send a patch for those one later.

OXPHOS changed the title ~~linalg - CPUdot~~ Linalg Refactor - CPU dot May 30, 2016

lambday reviewed May 30, 2016
View reviewed changes

lambday reviewed Jun 1, 2016
View reviewed changes

OXPHOS force-pushed the linalg branch from d216ecc to 6c945bb Compare June 1, 2016 20:22

lambday reviewed Jun 2, 2016
View reviewed changes

lambday reviewed Jun 7, 2016
View reviewed changes

OXPHOS added 2 commits June 8, 2016 00:09

fix style

177f5ea

update

b73728f

OXPHOS added 2 commits June 8, 2016 12:05

header

12318a1

gnu convert test case

d4b5d8b

lambday reviewed Jun 8, 2016
View reviewed changes

OXPHOS added 2 commits June 8, 2016 16:48

update

347e7bd

fix wrong path

be52bba

lambday reviewed Jun 9, 2016
View reviewed changes

lambday merged commit cf33090 into shogun-toolbox:feature/linalg_refactor Jun 9, 2016

OXPHOS mentioned this pull request Jun 9, 2016

Linalg - Vector Class #3277

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Linalg Refactor - CPU dot #3230

Linalg Refactor - CPU dot #3230

OXPHOS commented May 30, 2016

karlnapf commented May 30, 2016

lambday May 30, 2016 •

edited

lambday commented May 30, 2016

OXPHOS commented Jun 1, 2016

lambday commented Jun 1, 2016

OXPHOS commented Jun 1, 2016

lambday Jun 1, 2016 •

edited

OXPHOS commented Jun 2, 2016

lambday Jun 2, 2016

lambday Jun 2, 2016

lambday Jun 7, 2016

OXPHOS commented Jun 8, 2016

lambday commented Jun 8, 2016

OXPHOS commented Jun 8, 2016

lambday Jun 8, 2016

lambday Jun 9, 2016

lambday commented Jun 9, 2016


		CPUVector(const CPUVector<T> &vector);

		bool onGPU() { return false; }

Linalg Refactor - CPU dot #3230

Linalg Refactor - CPU dot #3230

Conversation

OXPHOS commented May 30, 2016

karlnapf commented May 30, 2016

lambday May 30, 2016 • edited

Choose a reason for hiding this comment

lambday commented May 30, 2016

OXPHOS commented Jun 1, 2016

lambday commented Jun 1, 2016

OXPHOS commented Jun 1, 2016

lambday Jun 1, 2016 • edited

Choose a reason for hiding this comment

OXPHOS commented Jun 2, 2016

lambday Jun 2, 2016

Choose a reason for hiding this comment

lambday Jun 2, 2016

Choose a reason for hiding this comment

lambday Jun 7, 2016

Choose a reason for hiding this comment

OXPHOS commented Jun 8, 2016

lambday commented Jun 8, 2016

OXPHOS commented Jun 8, 2016

lambday Jun 8, 2016

Choose a reason for hiding this comment

lambday Jun 9, 2016

Choose a reason for hiding this comment

lambday commented Jun 9, 2016

lambday May 30, 2016 •

edited

lambday Jun 1, 2016 •

edited