move scale to linalg #2744

SunilMahendrakar · 2015-02-27T21:13:19Z

towards #2717. Everything is not yet replaced.

karlnapf · 2015-02-28T23:19:31Z

src/shogun/statistics/QuadraticTimeMMD.cpp

@@ -1023,7 +1024,7 @@ SGVector<float64_t> CQuadraticTimeMMD::sample_null_spectrum_DEPRECATED(

 	/* when m=n, return m*MMD^2 instead */
 	if (m==n)
-		null_samples.scale(0.5);
+		linalg::scale(null_samples, null_samples, 0.5);


hey

just curious, can't there be a convenience method for the case where both arugments are the same?

hi,
We'll have to add a simple method. I'll add it.

SunilMahendrakar · 2015-03-01T09:19:13Z

@karlnapf please take a look.

lambday · 2015-03-04T05:54:54Z

src/shogun/mathematics/linalg/internal/implementation/Scale.h

+		{
+			for (int32_t i=0; i<A.vlen; i++)
+				B[i]=A[i]*alpha;
+		}


Some indentation issue is there.

lambday · 2015-03-04T06:14:14Z

Hi @SunilMahendrakar. Sorry for such a late reply. Have been caught up with something.

The PR looks good. I have some comments and it would be great if you could please address those - shouldn't take more than 5 mins. I know GSoC is not happening for us this summer which is depressing, but it would be great if we can finish at least those things that we started. Looking forward to it.

SunilMahendrakar · 2015-03-04T09:34:43Z

@lambday Sure!. GSoC would have been great but I'll be contributing anyways.

lambday · 2015-03-04T11:06:47Z

That's really nice to hear :) Looking forward.

SunilMahendrakar · 2015-03-04T20:05:52Z

@lambday done :) please take a look.

lambday · 2015-03-05T06:01:22Z

src/shogun/mathematics/linalg/internal/implementation/Scale.h

+			B[i]=A[i]*alpha;
+	}
+};
+


@SunilMahendrakar well what I meant here was to have an additional method that just takes raw pointer types and the length of the consecutive memory array.

static void compute(T* A, T* B, index_t len, T alpha) { // REQUIRE on null check for both A and B // the loop goes here }

So you write the loop there just once and make the SGMatrix and SGVector ones to call that method instead. That way, we'll be able to handle handle raw arrays as well (we can later simply add another wrapper which calls that with NATIVE backend). See what I mean?

@lambday yeah. so that we just have a REQUIRE there which calls this method to perform the actual computation.

Yeah. Keep the other requires there - they are useful. The null check REQUIRE is useful when people (hopefully mistakenly) pass scale(NULL, 1000, 0.5) in the raw ptr method.

lambday · 2015-03-05T06:12:58Z

Hi @SunilMahendrakar . This looks good. Just see my other comment.

A couple of other things I forgot to mention

as of now C++11 is not a hard requirement for Shogun, but it is so for linalg. So whenever using linalg methods, we should (at present) guard our code with HAVE_CXX0X or HAVE_CXX11. Otherwise compilation will fail on platforms that ironically still use prehistoric compilers. The rule of thumb is -
- If the linalg method has a native implementation, we should guard our code with HAVE_CXX0X or HAVE_CXX11. (we would get rid of this requirement soon!)
- If the linalg method doesn't have a native implementation and only available with external linalg libs, we should guard it with HAVE_LINALG_LIB (this takes care of C++11 guard as well)
- If the linalg method only has implementation against a particular backend, we should guard it for that particular lib (HAVE_EIGEN3 or HAVE_VIENNACL) alongwith C++11 guard.
Whenever we add a native implementation for some method, the purpose of that is to make it available even in absence of any external linalg libs. So in the wrapper, we should pull that method out of HAVE_LINALG_LIB guard. See that add method resides outside that guard.

SunilMahendrakar · 2015-03-05T07:51:25Z

@lambday thanks for the explanation. :)

SunilMahendrakar · 2015-03-06T05:17:02Z

@lambday it would be better if we first work towards removing HAVE_CXX11 first then move the calls to linalg, since if we want to prevent compilation failures we would have to add the guard all over the place starting from where we use it, i.e for things dependent on the method where we use it and we have 30-40 scale calls alone. Its easy for outermost methods like unit tests. Since we are going to lose it anyway lets start working towards losing it.

lambday · 2015-03-06T06:55:12Z

@SunilMahendrakar that totally makes sense. We want the build to fail in cmake itself in absence of c++11.

However, until that happens, I'd suggest that replacing everything with linalg calls can be done later, in separate patches. As of now, we'd be fine just adding it to linalg and having unit-tests. So don't replace everything yet - let's first get native scale merged.

lambday · 2015-03-06T08:39:55Z

Please rebase.

SunilMahendrakar · 2015-03-06T11:59:14Z

@lambday done please take a look.

iglesias · 2015-03-08T10:51:41Z

src/shogun/mathematics/linalg/internal/implementation/Scale.h

+	static void compute(SGMatrix<T> A, SGMatrix<T> B, T alpha)
+	{
+		REQUIRE((A.num_rows==B.num_rows)&&(A.num_cols==B.num_cols),
+			"Dimensions of A(%dx%d) and B(%dx%d) don't match.\n",


I think we can skip the \n.

@iglesias I personally like the \n in the end - as per our wiki on Assertions -

Use proper English (Start with capital letters, write sentences)

Don't forget \n at the end.

Make sure that you don't cause segfaults in your assertions, check bounds, pointer valid, etc

Although it is just a matter of choice.

iglesias · 2015-03-08T10:52:44Z

It looks good to me, just very minor comments above. @lambday, ready to merge?

lambday · 2015-03-09T07:04:34Z

tests/unit/mathematics/linalg/Scale_unittest.cc

+	for (int32_t i=0; i<9; i++)
+		EXPECT_NEAR(alpha*A[i], B[i], 1e-15);
+}
+


@SunilMahendrakar could you please add a couple of these tests (one on SGMatrix and another on SGVector) that does the scale operation in place? This way, linalg::scale(A, alpha) is also tested.

Actually, it makes sense to have unit tests for in place scale operation for all the backends - since we are exposing it for all of 'em.

lambday · 2015-03-09T07:07:27Z

@SunilMahendrakar great job! Thanks for the patch :) Merging. If you could please add two more unit-tests that I mentioned, we can consider the matter close :)

move scale to linalg

SunilMahendrakar · 2015-03-09T07:32:43Z

@lambday buildbots are going crazy. test failing unrelated?

lambday · 2015-03-09T08:28:43Z

@SunilMahendrakar yeah SerializationJSON is a long term serial killer. We should rename buildbot to blamebot.

SunilMahendrakar mentioned this pull request Feb 28, 2015

make use of linalg within Shogun #2747

Open

karlnapf reviewed Feb 28, 2015
View reviewed changes

lambday reviewed Mar 4, 2015
View reviewed changes

lambday reviewed Mar 5, 2015
View reviewed changes

iglesias reviewed Mar 8, 2015
View reviewed changes

SunilMahendrakar added 2 commits March 9, 2015 11:52

added native scale impl to linalg

1e9bfad

fixed minor errors.

64311a0

lambday reviewed Mar 9, 2015
View reviewed changes

lambday added a commit that referenced this pull request Mar 9, 2015

Merge pull request #2744 from SunilMahendrakar/scale

136b497

move scale to linalg

lambday merged commit 136b497 into shogun-toolbox:develop Mar 9, 2015

SunilMahendrakar deleted the scale branch March 9, 2015 08:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

move scale to linalg #2744

move scale to linalg #2744

SunilMahendrakar commented Feb 27, 2015

karlnapf Feb 28, 2015

SunilMahendrakar Mar 1, 2015

SunilMahendrakar commented Mar 1, 2015

lambday Mar 4, 2015

lambday commented Mar 4, 2015

SunilMahendrakar commented Mar 4, 2015

lambday commented Mar 4, 2015

SunilMahendrakar commented Mar 4, 2015

lambday Mar 5, 2015

SunilMahendrakar Mar 5, 2015

lambday Mar 5, 2015

lambday commented Mar 5, 2015

SunilMahendrakar commented Mar 5, 2015

SunilMahendrakar commented Mar 6, 2015

lambday commented Mar 6, 2015

lambday commented Mar 6, 2015

SunilMahendrakar commented Mar 6, 2015

iglesias Mar 8, 2015

lambday Mar 9, 2015

iglesias commented Mar 8, 2015

lambday Mar 9, 2015

lambday Mar 9, 2015

SunilMahendrakar Mar 9, 2015

lambday commented Mar 9, 2015

SunilMahendrakar commented Mar 9, 2015

lambday commented Mar 9, 2015

move scale to linalg #2744

move scale to linalg #2744

Conversation

SunilMahendrakar commented Feb 27, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SunilMahendrakar commented Mar 1, 2015

Choose a reason for hiding this comment

lambday commented Mar 4, 2015

SunilMahendrakar commented Mar 4, 2015

lambday commented Mar 4, 2015

SunilMahendrakar commented Mar 4, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lambday commented Mar 5, 2015

SunilMahendrakar commented Mar 5, 2015

SunilMahendrakar commented Mar 6, 2015

lambday commented Mar 6, 2015

lambday commented Mar 6, 2015

SunilMahendrakar commented Mar 6, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iglesias commented Mar 8, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lambday commented Mar 9, 2015

SunilMahendrakar commented Mar 9, 2015

lambday commented Mar 9, 2015