Simple MC Integrate Feature #163

surajchoubey · 2021-06-13T13:03:59Z

Simple MC Integrate is a new feature which is proposed to be added to GeomScale/volume_approximation
It involves include/integration/simple_MC_integration.hpp and for the tests tests/simple_mc_integration.cpp with 6 tests written for ( all for H-rrepresentations) :

Cubes
Rectangles
Simplices
Product Simplices
Cross Polytopes
Birkhoff Polytopes

Link to the blog post: https://surajchoubey.github.io/gsoc21/simple-mc-integration/

Link to the tests: https://github.com/surajchoubey/latte-integrale-checks

Testing has been done using Latte-Integrale Software.

References:
LL = Lower Limits
UL = Upper Limits
N = unsigned integer number of sample points

simple_MC_integration.hpp involves 5 functions :

template
< typename Point = Point, typename NT = NT >
bool valid_limits(Point LL, Point UL)

To check if given two Points represent proper integration limits LL(i) <= UL(i) ; 0<i<LL.dimension() and LL.dimension() should be equal to UL.dimension()

template
<  typename Point = Point, typename NT = NT >
Point init_limit(Limit L, Uint dim)

This function is designed for simple_mc_integrate() to tailor a hyper-rectangle using the assigned limits which is supposed to be the subspace for performing MC Algorithm.
3.

template
<  typename Point = Point, typename NT = NT >
void initiate_unit_limits(Point& LL, Point& UL, int dim)

This is function is designed to initiate unit limits from [-1,1]^n as default integration limits.

template 
<
    typename WalkType = BallWalk,
    typename Polytope = HPOLYTOPE,
    typename RNG = RandomNumberGenerator,
    typename NT = NT,
    typename Functor
>
NT simple_mc_polytope_integrate(Functor Fx, 
                                Polytope &P, 
                                Uint N = 10000, 
                                volumetype voltype = SOB, 
                                int walk_length = 1, 
                                NT e = 0.1, 
                                Point Origin = pt)

This function is designed to hold convex bodies in general for integration limits.
Type of random walk can be entered manually by user ( default set to BallWalk )
Polytope(or a convex body) is to be supplied meant to be supplied as a parameter in the function .
Volume of subspace is calculated using volesti volume libraries.

template
<
    typename WalkType = BallWalk,
    typename RNG = RandomNumberGenerator,
    typename NT = NT,
    typename Functor
>
NT simple_mc_integrate (Functor Fx, 
                        Uint dim, 
                        Uint N = 10000, 
                        volumetype voltype = SOB, 
                        Limit LowLimit = lt, 
                        Limit UpLimit = lt, 
                        int walk_length = 10, 
                        NT e = 0.1)

This function is designed to hold integration limits in general
A hyper-rectangle is tailored from the limits entered which is supposed to be subspace for performing MC Algorithm.

…/volume_approximation into simple-MC-integrate

papachristoumarios

Thank you for your PR!

I want to highlight some issues that need to be addressed before merging it, which I believe would need a considerable amount of work:

Coding style: I have noticed that the coding style is inconsistent across all files (e.g. whitespaces, clauses, names of identifiers etc.), and even within functions. I suggest you look at the current code style and format your code accordingly.
Lack of tests: The current code that is in the test directory contains no tests. By tests I mean taking the output of two different methods (e.g. naive method and LattE's method and comparing them). Also, you should do tests that check the values of known integrals over simple domains.
Redundant code: There is redundant code across this PR. Please have a better look at the codebase and more specifically include/ode_solvers/oracle_functors.cpp. Also, the same function is defined multiply with no apparent reason.
Suppose that you are given a distribution p(x) = f(x) / Z where Z is the integral of f(x) over the domain K (i.e. the normalization constant of p(x)), and a function g(x) defined on K. Then given samples X_1, ..., X_n ~ p and their transformations Y_i = g(X_i) their average, i.e. sum(Y_1, ..., Y_n) / n should converge to (1 / Z) * integral_K f(x) g(x) dx = integral_K p(x) g(x) dx where in general Z does not equal vol(K). When p is the uniform distribution over K then Z = vol(K) and f(x) = 1, so the average converges to (1 / vol(K)) * integral_K g(x) dx. That said, I believe that #L175 in simple_MC_integration.hpp calculates the wrong quantity.

Please make the appropriate changes and let us know when to re-review.

Thank you in advance

include/integration/simple_MC_integration.hpp

papachristoumarios · 2021-06-15T23:23:02Z

include/integration/simple_MC_integration.hpp

+// To return ||X||^2 for a VT
+NT normSquared(VT& X){
+    NT sum=0;
+    for( int i=0; i < X.rows() ; i++ ){


Avoid writing in loops things that can be vectorized.

include/integration/simple_MC_integration.hpp

test/simple_mc_integration.cpp

papachristoumarios · 2021-06-16T13:24:38Z

test/simple_mc_integration.cpp

+    VT newOrigin(4);
+    newOrigin << 0.2, -0.7, 0.96, -0.79;
+    HPOLYTOPE HP1 = generate_cube<HPOLYTOPE>(4, false);
+    SimpleMCPolytopeIntegrate(expXY4D, HP1, 15000, SOB , newOrigin);


Do you need to add CHECK() here?

vissarion

Thanks @surajchoubey for this PR! In general it is in good shape, I have some comments regarding the interface to inline with the interfaces of similar functions (e.g. volume). Also the code styling needs some polishing.

include/integration/simple_MC_integration.hpp

papachristoumarios

Thank you for the PR! In general, the PR has been improved a lot since the last iteration.
I have left some comments to address and I would like to highlight that you should be more careful with the code structure between header and source files. I have left you some advice regarding how to address them.

include/integration/simple_MC_integration.hpp

papachristoumarios · 2021-06-28T19:02:06Z

include/integration/simple_MC_integration.hpp

+enum volType { CB , CG , SOB }; // Volume type for polytope
+
+// To check if two n-dimensional points ensure valid limits in integration 
+bool validLimits(Point LL, Point UL){


Should this method throw an exception instead of giving an output to stdout?

This is meant to check if UL[i] >= LL[i]

Thank you! What I meant was that these errors should be printed on stderr, which you do below.

include/integration/simple_MC_integration.hpp

papachristoumarios · 2021-06-28T19:04:39Z

include/integration/simple_MC_integration.hpp

+    typename WalkType=BallWalk,
+    typename Functor
+>
+void simple_mc_integrate(Functor Fx, Uint N ,Point LL, Point UL, int walk_length=10, NT e=0.1){


Should this function take a generic polytope as an argument?
You can create an extra function which is more user-friendly: ie it takes Point LL and Point UL and then calls this function.

Moreover, please check the spacing in the code which is not consistent.

Well not actually simple_mc_integrate() is designed to take integration limits as Points. (This is come up with thought to ease the user)

simple_mc_polytope_integrate() is designed to take generic polytope as argument.

include/integration/simple_MC_integration.hpp

test/simple_mc_integration.cpp

papachristoumarios

Good work!

include/integration/simple_MC_integration.hpp

vissarion · 2021-07-26T12:42:38Z

test/simple_mc_integration.cpp

+}
+
+template <typename NT>
+void call_test_simple_mc_integration_over_rectangles() {


this contains integration over segments too, so this could be included in the name

test/simple_mc_integration.cpp

vissarion · 2021-07-26T12:57:43Z

include/integration/simple_MC_integration.hpp

+typedef Cartesian<NT> Kernel;
+typedef typename Kernel::Point Point;
+typedef std::vector<Point> Points;
+typedef HPolytope<Point> HPOLYTOPE;


this is not a special type so calling it Hpolytope instead of capitilize, is OK

changed HPOLYTOPE -> Hpolytope

include/integration/simple_MC_integration.hpp

vissarion · 2021-07-26T14:05:46Z

include/integration/simple_MC_integration.hpp

+
+    switch (voltype) {
+    case CB:     
+        volume = volume_cooling_balls <BallWalk, RNG, Polytope> (P, e, walk_length).second; 


the walk parameter here should be part of the interface i.e. a new template e.g. VolumeWalkType that should be passed here instead of BallWalk

SOB/CG are fine to good to go on this.
What to do the CG Walktype? Cooling Gaussians seem to use GaussianFamily Walks only.

vissarion · 2021-07-26T14:06:58Z

test/simple_mc_integration.cpp

+
+	Limit LL1{0.5};
+	Limit UL1{10};
+	integration_value = simple_mc_integrate <BilliardWalk> (logx_natural_1D, 1, 100000, CB, LL1, UL1);


100K is too much I think, tests are slow, do you get good enough (for tests) results with 1K ?

I am doing 1K mostly everywhere and 10K where dimensions are greater than 7.

I checked the results and I could see satisfiable relative errors.

Used 100K once only because I created some weird 1D Polytope sampling space

surajchoubey and others added 13 commits June 6, 2021 17:14

Simple MC integration 1st commit

18582ee

.gitignore updated test/build

386a253

added build to .gitignore

a96f4eb

Simple MC integration hpp draft

09c0109

removed testing mess

ee6e631

minor VT reps fixes

671d414

minor eigen fixes2

6c4f676

cleaning + volType enum added

cc3cf5f

updated .gitignore for build

8e4a377

tests ready for check

37d6104

Merge branch 'GeomScale:develop' into simple-MC-integrate

2949acb

all 6 tests passing

b49256f

Merge branch 'simple-MC-integrate' of https://github.com/warmachine86…

e7cea91

…/volume_approximation into simple-MC-integrate

vissarion added the enhancement label Jun 14, 2021

vissarion requested review from papachristoumarios, TolisChal and vissarion and removed request for papachristoumarios and TolisChal June 14, 2021 08:00

papachristoumarios suggested changes Jun 16, 2021

View reviewed changes

surajchoubey added 2 commits June 19, 2021 19:12

removed user-def dir

1e6471e

Cleaned up + fixed petty errors

1b0cd55

vissarion reviewed Jun 22, 2021

View reviewed changes

surajchoubey added 5 commits June 23, 2021 11:44

implemented uni bill walks + removed hmc

364c9eb

simple mc uniform walks completed

11783b1

Vector<Point> to store points added

78c83d2

added feature for VPOLYTOPE

9d7a5d5

removed inbuilt sampling + added uniform walks

d6f0bc2

put walk_len + e to params

e3b892f

papachristoumarios reviewed Jun 28, 2021

View reviewed changes

surajchoubey added 8 commits June 30, 2021 02:13

Added CHECK() + test_values()

85963a5

Added [-1,1]^n as default limits

200f019

added tests for simple_mc_integrate

3176dd2

added Limits datatype

91373bf

minor fix in naming tests simple mc

10cc6e8

minor fixes in dims

977541f

fixed polytope dim match

92503ff

fixed polytope dim-2

8385366

papachristoumarios reviewed Jul 2, 2021

View reviewed changes

test/simple_mc_integration.cpp Show resolved Hide resolved

surajchoubey added 4 commits July 4, 2021 00:03

fixed indent

740279d

added tests birkhoff, prod-simplex, simplex, cross

e26e424

testing wrt latte

e4e6da4

fixed walks

17f5b2f

papachristoumarios approved these changes Jul 9, 2021

View reviewed changes

vissarion reviewed Jul 26, 2021

View reviewed changes

include/integration/simple_MC_integration.hpp Outdated Show resolved Hide resolved

vissarion reviewed Jul 26, 2021

View reviewed changes

include/integration/simple_MC_integration.hpp Show resolved Hide resolved

vissarion approved these changes Jul 26, 2021

View reviewed changes

surajchoubey and others added 3 commits August 1, 2021 20:35

lowered N for tests + VolumeWalkType added

5b35e7d

fixed rectangle tests

bb0b394

Merge branch 'develop' into simple-MC-integrate

808a5f8

vissarion merged commit 3a7d072 into GeomScale:develop Oct 3, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simple MC Integrate Feature #163

Simple MC Integrate Feature #163

surajchoubey commented Jun 13, 2021 •

edited

Loading

papachristoumarios left a comment

papachristoumarios Jun 15, 2021

papachristoumarios Jun 16, 2021

vissarion left a comment

papachristoumarios left a comment

papachristoumarios Jun 28, 2021

surajchoubey Jun 29, 2021

papachristoumarios Jul 2, 2021

papachristoumarios Jun 28, 2021

surajchoubey Jun 29, 2021

papachristoumarios left a comment

vissarion Jul 26, 2021

vissarion Jul 26, 2021 •

edited

Loading

surajchoubey Aug 1, 2021

vissarion Jul 26, 2021

surajchoubey Aug 1, 2021

vissarion Jul 26, 2021

surajchoubey Aug 1, 2021 •

edited

Loading

Simple MC Integrate Feature #163

Simple MC Integrate Feature #163

Conversation

surajchoubey commented Jun 13, 2021 • edited Loading

papachristoumarios left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vissarion left a comment

Choose a reason for hiding this comment

papachristoumarios 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

papachristoumarios left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vissarion Jul 26, 2021 • edited Loading

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

surajchoubey Aug 1, 2021 • edited Loading

Choose a reason for hiding this comment

surajchoubey commented Jun 13, 2021 •

edited

Loading

vissarion Jul 26, 2021 •

edited

Loading

surajchoubey Aug 1, 2021 •

edited

Loading