Added Stan Math Support to Shogun #4216

FaroukY · 2018-03-25T05:54:53Z

I've added the ability to now use the Stan Math library in Shogun. I've also written a unit test to make sure it compiles and works properly (The unit test is a perceptron learning algorithm with gradient descent)

The CmakeLists.txt was changed to add the include directories of stan headers, and the last file is the unit test.

vigsterkr · 2018-03-25T07:11:02Z

cmake/external/StanMath.cmake

@@ -0,0 +1,18 @@
+include(ExternalProject)
+ExternalProject_Add(


you should fix the commit sha, instead of following the master HEAD. basically set
GIT_TAG and set it to the last stable release of stan

I set up the GIT_TAG as required

vigsterkr · 2018-03-25T07:11:41Z

src/shogun/CMakeLists.txt

+include(external/StanMath)
+SHOGUN_INCLUDE_DIRS(SCOPE PUBLIC SYSTEM
+  $<BUILD_INTERFACE:${STAN_INCLUDE_DIR_STAN_MATH}>
+  $<INSTALL_INTERFACE:include/shogun/lib/external/stan1>


what's the stan1 stan2 stan3 stands for?

Changed the names to be more descriptive.

vigsterkr · 2018-03-25T07:12:49Z

great! there are some minor improvements needed

FaroukY · 2018-03-29T04:22:46Z

@vigsterkr Done with the required improvements.

vigsterkr · 2018-03-29T08:00:47Z

@FaroukY lemme check why do the CIs fail

vigsterkr · 2018-03-29T08:02:19Z

@FaroukY does this work for you locally? maybe because boost is in your include paths? as you can see from here this does not compile:
https://travis-ci.org/shogun-toolbox/shogun/jobs/359680795#L1970

stan misses boost... lemme try to pull these commits and see locally what's happening and get back to you

vigsterkr · 2018-03-29T09:08:29Z

indeed the problem is that - i've just tested this with a machine that has boost installed and it worked fine... i'll test and try to come up with a fix in a clean docker container

vigsterkr

there's definitely a problem with INSTALL files missing.

vigsterkr · 2018-03-29T09:08:58Z

src/shogun/CMakeLists.txt

+include(external/StanMath)
+SHOGUN_INCLUDE_DIRS(SCOPE PUBLIC SYSTEM
+  $<BUILD_INTERFACE:${STAN_INCLUDE_DIR_STAN_MATH}>
+  $<INSTALL_INTERFACE:include/shogun/lib/external/Stan>


who and what will install those the folder here?

i would actually start a new dir: include/shogun/third_party and start putting stuff there that are external... we have too many things in lib

vigsterkr · 2018-03-29T09:09:02Z

src/shogun/CMakeLists.txt

+)
+SHOGUN_INCLUDE_DIRS(SCOPE PUBLIC SYSTEM
+  $<BUILD_INTERFACE:${STAN_INCLUDE_DIR_BOOST}>
+  $<INSTALL_INTERFACE:include/shogun/lib/external/Stan_Boost>


who and what will install those the folder here?

vigsterkr · 2018-03-29T09:09:05Z

src/shogun/CMakeLists.txt

+)
+SHOGUN_INCLUDE_DIRS(SCOPE PUBLIC SYSTEM
+  $<BUILD_INTERFACE:${STAN_INCLUDE_DIR_CVODES}>
+  $<INSTALL_INTERFACE:include/shogun/lib/external/Stan_Cvodes>


who and what will install those the folder here?

can you please rebase this over latest develop and change this to include/shogun/third_party/Stan_Cvodes

vigsterkr · 2018-03-29T09:41:34Z

cmake/external/StanMath.cmake

+add_dependencies(libshogun StanMath)
+
+set(STAN_INCLUDE_DIR_STAN_MATH ${CMAKE_BINARY_DIR}/StanMath/src/StanMath)
+set(STAN_INCLUDE_DIR_BOOST ${CMAKE_BINARY_DIR}/StanMath/src/StanMath/lib/boost_1.66.0)


this should be boost_1.64.0

FaroukY · 2018-04-01T15:59:10Z

@vigsterkr I think the problem is indeed that I had boost in my path. Can you share the Dockerfile that you used to test this? I want to make sure that I get it working on a general machine before the I push my commits.

karlnapf · 2018-04-02T11:10:34Z

tests/unit/stanmath/StanPerceptronTest_unittest.cc

+Since we can find weights from [1 1 1]^T to [1 1]^T in a perceptron,
+this error should be very close to zero after 100 epochs.
+*/
+TEST(StanPerceptronTest, sample_perceptron)


while I like this test, I think we should have something super simple. Like the gradient of the gaussian kernel wrt to bandwidth, left/right argument. Something that we know in closed form.

But please de-prioritise the test until all the cmake stuff is correct

this test is very fine, lets concentrate on the actual inclusion of the lib

karlnapf · 2018-04-02T11:11:19Z

tests/unit/stanmath/StanPerceptronTest_unittest.cc

+  }
+
+  //Error should be very close to 0.0
+  EXPECT_NEAR(last_error, 0.0, 1e-6);


this might fail randomly if the data is not separable.

Yes, but in this case, there is only one data point ( [1 1 1]^T) so the data is linearly separable (in an infinite number of ways) so it shouldn't fail no?

karlnapf · 2018-04-02T11:11:36Z

tests/unit/stanmath/StanPerceptronTest_unittest.cc

+  std::random_device rd{};
+  std::mt19937 gen{rd()};
+  normal_distribution<> d{0,1};
+  Matrix<var, 2, 3> W1;


use N and D

I am not sure what you mean here. What's N and D? Do you mean the variable names?

karlnapf · 2018-04-02T11:11:42Z

tests/unit/stanmath/StanPerceptronTest_unittest.cc

+  std::mt19937 gen{rd()};
+  normal_distribution<> d{0,1};
+  Matrix<var, 2, 3> W1;
+  for(int i=0; i<2; ++i)


Will change it.

karlnapf · 2018-04-02T11:12:10Z

tests/unit/stanmath/StanPerceptronTest_unittest.cc

+    {
+      error += (outputs(i,0)-1)*(outputs(i,0)-1); //square error
+    }
+    error.grad(); //Calculate the gradient


superfluous comment

Will get rid of it.

karlnapf · 2018-04-02T11:12:59Z

Quite cool to have stan in. Let's get going with this patch!
There are some open requests by viktor @FaroukY

vigsterkr · 2018-04-03T08:27:26Z

@FaroukY i've use the same docker image that is being used for travis CI jobs: shogun/shogun-dev

vigsterkr · 2018-04-03T08:31:06Z

@FaroukY basically the problems are the following:

you are using a system where you already have libboost installed (probably under /usr/include...)
you have not tested the cmake script with the given GIT_TAG (the boost versions are different)
the way you handle STAN_INCLUDE_DIR*... those are not going to automagically install themselves under include/shogun/..... you need to take care of that.

vigsterkr · 2018-04-03T08:39:38Z

tests/unit/stanmath/StanPerceptronTest_unittest.cc

@@ -0,0 +1,73 @@
+#include <gtest/gtest.h>
+#include <stan/math.hpp>


i'm not so sure if this is the best way... namely that you are including stan like this.
meaning what if for some reason stan/math.hpp is in the include path already... probably the -isystem inclusion will take care of this, but maybe it'd be better that we explicitly specify in the include path that it's shogun's third-party include path... on the other hand probably this will actually break everything else within stan's headers....

it's more important actually that stan usage (or a model that is actually using stan) is being tested from a libshogun example. namely that the cmake target flags are set correctly when shogun is being used from a third-party lib, or just a simple CLI. note that it requires that either you:

either include stan only in cpp files of shogun, or

you expose the include flags in ShogunTarget.cmake; currently it's PUBLIC SYSTEM which should (? does it really? should be tested) reflect the correct exposure of include flags.... but i'm not so sure if we want to expose stan to the public, i would presume it's safe to think that we only use stan in the implementation of the models.

vigsterkr · 2018-04-05T12:53:49Z

@FaroukY any update?

FaroukY · 2018-04-05T15:49:35Z

@vigsterkr In progress. I just set up my environment with Docker (was new to it) and started changing the cmake and fixing the problems. I am a bit tied down these coming few days but I am working a bit on it everyday. Also was handling the other prs.

FaroukY · 2018-04-05T22:47:48Z

@vigsterkr It appears that the only thing that was wrong here is that I miswrote the boost directory from 1.66 to 1.64. Once I changed 1.66 to 1.64, the build compiled and finished. For now, I will:

Change the directory of Stan from include/shogun/lib/external/Stan to include/shogun/third_party/Stan
Retest and push again on this branch.
Address the changes in the test mentioned above.

Also, it looks like there are two jobs in Travis which timed out (one during the testing phase) so we can assume the build worked for it.

vigsterkr · 2018-04-06T10:29:22Z

@FaroukY there's still a major part missing: installation of the Stan headers in the right folder. the tests are working because you rely on the fact that everything is available in build time. but once you ship this, the stan related directories are not going to be available as you dont copy them with make install

FaroukY · 2018-04-06T15:50:08Z

@vigsterkr I am new to cmake so I'm a bit confused here. When I make and make install on my Docker machine, this seems to build and install correctly. What do you mean by "but once you ship this, the stan related directories are not going to be available" and how can I test this.

Also, any guidance on what the next steps might be would be super helpful since I've been reading on Cmake for quite a few days now and I'm not sure where to go from here.

vigsterkr · 2018-04-10T08:53:00Z

@FaroukY ok so when you do make install do you have files under /usr/local/include/shogun/lib/external/Stan ? because i doubt so... as there's nothing instructing cmake to install those files anywhere.

FaroukY · 2018-04-16T07:24:52Z

@vigsterkr Yes, just tried it. I have the file directories /usr/local/include/shogun/lib/external/Stan, /usr/local/include/shogun/lib/external/Stan_Boost, and /usr/local/include/shogun/lib/external/Stan_Cvodes so I think cmake automatically puts them there.

FaroukY · 2018-04-16T07:35:02Z

@vigsterkr Actually it was my mistake. I forgot to commit the changes to this branch facepalm so now it has the Install commands. Should be okay now. Installs the 3 libraries into /usr/local/include/shogun/lib/external/*

vigsterkr

there has been some changes in develop... see my comments

vigsterkr · 2018-04-20T09:32:16Z

src/shogun/CMakeLists.txt

+
+SHOGUN_INCLUDE_DIRS(SCOPE PUBLIC SYSTEM
+  $<BUILD_INTERFACE:${STAN_INCLUDE_DIR_STAN_MATH}>
+  $<INSTALL_INTERFACE:include/shogun/lib/external/Stan>


can you please rebase this over latest develop and change this to include/shogun/third_party/Stan

vigsterkr · 2018-04-20T09:32:25Z

src/shogun/CMakeLists.txt

+)
+SHOGUN_INCLUDE_DIRS(SCOPE PUBLIC SYSTEM
+  $<BUILD_INTERFACE:${STAN_INCLUDE_DIR_BOOST}>
+  $<INSTALL_INTERFACE:include/shogun/lib/external/Stan_Boost>


can you please rebase this over latest develop and change this to include/shogun/third_party/Stan_Boost

vigsterkr · 2018-04-20T09:32:44Z

src/shogun/CMakeLists.txt

+)
+SHOGUN_INCLUDE_DIRS(SCOPE PUBLIC SYSTEM
+  $<BUILD_INTERFACE:${STAN_INCLUDE_DIR_CVODES}>
+  $<INSTALL_INTERFACE:include/shogun/lib/external/Stan_Cvodes>


can you please rebase this over latest develop and change this to include/shogun/third_party/Stan_Cvodes

vigsterkr · 2018-04-20T09:36:22Z

cmake/external/StanMath.cmake

+set(STAN_INCLUDE_DIR_CVODES ${CMAKE_BINARY_DIR}/StanMath/src/StanMath/lib/cvodes_2.9.0/include)
+
+
+INSTALL( DIRECTORY ${STAN_INCLUDE_DIR_STAN_MATH} DESTINATION include/shogun/lib/external/Stan )


instead of this you should actually copy these files into ${THIRD_PARTY_INCLUDE_DIR}/Stan

remove these installs.... if you do a cmake copy thing then you will not have problems with the logs in travis.

check how COMMAND ${CMAKE_COMMAND} -E copy_if_different is being used in shogun

and the best place to do this copy would be ExternalProject_Add's INSTALL_COMMAND

FaroukY · 2018-05-04T13:31:03Z

@vigsterkr Did the required changes and its not rebased with develop. However, I had a look at travis and its failed because the log messages of installing Stan is exceeding 4Mb (Since there are many files that are being installed). But the build itself is building fine.

vigsterkr · 2018-05-04T14:34:37Z

@FaroukY yeah first we need a rebase over develop and then fix those things i've mentioned i'll think about how to fix travis limitations.

… the internal boost and cvodes that stan uses

… HEAD

FaroukY · 2018-05-04T19:09:55Z

@vigsterkr Did the required changes and the problem is not there anymore. However, I'm failing one of the tests: 1 - unit-TrainedModelSerializationTest (SEGFAULT) even though my PR has no relation to that test. Any ideas here?

vigsterkr · 2018-05-04T19:13:11Z

@FaroukY the segfault is unrelated, there's currently a bug in develop branch, so we can ignore that

vigsterkr · 2018-05-04T19:17:18Z

@FaroukY please check/fix the errors that clang-format reports, see: https://travis-ci.org/shogun-toolbox/shogun/jobs/375012262#L722

FaroukY · 2018-05-05T00:11:50Z

@vigsterkr Done, clang-format passes now.

iglesias · 2018-05-06T09:56:51Z

tests/unit/StanMath/StanPerceptronTest_unittest.cc

+In this example, we want to learn the weights W such that the square
+Error loss from the output of the perceptron to [1 1]^T is minimized.
+Since we can find weights from [1 1 1]^T to [1 1]^T in a perceptron,
+this error should be very close to zero after 100 epochs.


Neat job documenting!

iglesias · 2018-05-06T09:59:26Z

tests/unit/StanMath/StanPerceptronTest_unittest.cc

+	std::mt19937 gen{rd()};
+	normal_distribution<> d{0, 1};
+	Matrix<var, 2, 3> W1;
+	for (int i = 0; i < 2; ++i)


No need to update, just informational. A small improvement would be to query the matrix dimensions instead of hardcoding them in the loops.

vigsterkr · 2018-05-06T10:57:56Z

@FaroukY perfect! merging!

vigsterkr requested changes Mar 25, 2018

View reviewed changes

vigsterkr approved these changes Mar 29, 2018

View reviewed changes

vigsterkr requested changes Mar 29, 2018

View reviewed changes

vigsterkr reviewed Mar 29, 2018

View reviewed changes

karlnapf reviewed Apr 2, 2018

View reviewed changes

vigsterkr reviewed Apr 3, 2018

View reviewed changes

vigsterkr requested changes Apr 20, 2018

View reviewed changes

FaroukY force-pushed the add_Stan_math branch from 464d513 to f60500b Compare May 4, 2018 12:36

FaroukY added 11 commits May 4, 2018 14:16

Added the stan library include directories into CMakeLists as well as…

2e93edd

… the internal boost and cvodes that stan uses

Built a StanMath.cmake file to be used in the CMakelists.txt in the src

abd8a31

updating data submodule to latest

823a71a

Added the latest stable version of Stan math instead of fetching from…

46dac39

… HEAD

Changed stan1,2,3 to be more expressive of what the directories have

9053ae6

Changed the cmake boost file from 66 to 64 since it was a typo

86b8f59

Added installation requirements

b636230

Fixed the Cmake file for stan

651bec1

Changed CMakeLists so that Stan installs in third_party instead of lib

ff3e699

Added the Stan test and removed unnecciary comments

ef0a69b

removed INSTALL and replaced it with copy_if_different

9e44ffd

FaroukY force-pushed the add_Stan_math branch from 7e65749 to 9e44ffd Compare May 4, 2018 18:18

Rebased with data HEAD

d2dca46

Fixed clang-format style check

910d32c

iglesias reviewed May 6, 2018

View reviewed changes

vigsterkr merged commit 49e35df into shogun-toolbox:develop May 6, 2018

ktiefe pushed a commit to ktiefe/shogun that referenced this pull request Jul 30, 2019

Added Stan Math Support to Shogun (shogun-toolbox#4216)

7b4a348

		@@ -0,0 +1,18 @@
		include(ExternalProject)
		ExternalProject_Add(

		@@ -0,0 +1,73 @@
		#include <gtest/gtest.h>
		#include <stan/math.hpp>

		set(STAN_INCLUDE_DIR_CVODES ${CMAKE_BINARY_DIR}/StanMath/src/StanMath/lib/cvodes_2.9.0/include)


		INSTALL( DIRECTORY ${STAN_INCLUDE_DIR_STAN_MATH} DESTINATION include/shogun/lib/external/Stan )

Added Stan Math Support to Shogun #4216

Added Stan Math Support to Shogun #4216

Conversation

FaroukY commented Mar 25, 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

vigsterkr commented Mar 25, 2018

FaroukY commented Mar 29, 2018

vigsterkr commented Mar 29, 2018

vigsterkr commented Mar 29, 2018

vigsterkr commented Mar 29, 2018

vigsterkr 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

FaroukY commented Apr 1, 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

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

karlnapf commented Apr 2, 2018

vigsterkr commented Apr 3, 2018

vigsterkr commented Apr 3, 2018

Choose a reason for hiding this comment

vigsterkr commented Apr 5, 2018

FaroukY commented Apr 5, 2018

FaroukY commented Apr 5, 2018

vigsterkr commented Apr 6, 2018

FaroukY commented Apr 6, 2018 • edited

vigsterkr commented Apr 10, 2018

FaroukY commented Apr 16, 2018

FaroukY commented Apr 16, 2018

vigsterkr 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

FaroukY commented May 4, 2018

vigsterkr commented May 4, 2018

FaroukY commented May 4, 2018

vigsterkr commented May 4, 2018

vigsterkr commented May 4, 2018

FaroukY commented May 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vigsterkr commented May 6, 2018

FaroukY commented Apr 6, 2018 •

edited