HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaModerate

Pi by Monte-Carlo

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
montecarlostackoverflow

Problem

I was inspired by this SO post to investigate
a good Java8 way to calculate Pi based on simulation.

I used a similar task to learn about parallel programming on both CUDA,
and Intel Xeon Phi processors. Those systems are more geared for
parallel programming, but I felt inclined to apply it to 'regular' Java
anyway.

Wikipedia has an small section showing
this, and
includes the following graphic:

The following code performs the above simulation:

```
import java.util.Arrays;
import java.util.Random;

/**
* Approximate the value of Pi by using a Monte-Carlo simulation for the area of a circle of radius 1.
*
* @author rolf
*
*/
public class PiGuess {

private static final ThreadLocal LOCAL_RANDOM = new ThreadLocal() {
protected Random initialValue() {
return new Random();
};
};

private static final int CPU_COUNT = Runtime.getRuntime().availableProcessors();

/**
* Split a number of samples as evenly as possible over the number of available processors.
* @param samples the samples to split
* @return an array containing the number of samples to process on each processor.
*/
private static final long[] apportion(final long samples) {
int core = CPU_COUNT;
final long[] portions = new long[core];
long remaining = samples;

while (core > 0) {
final long part = (remaining - 1 + core) / core;
core--;
portions[core] = part;
remaining -= part;
}
return portions;
}

/**
* Calculate the approximate area of a circle (radius 1.0) based on a sample system on a single quadrant of the circle.
* A parallel mechanism is used to improve performance.
* @param samples the number of samples to take
* @return the area of the circle.
*/
public static final double sampleCircleArea(final long samples) {

/*
Monte-Carlo simulation

Solution

The code and documentation look fine in general, so this review will focus on minor optimizations on a per-method basis.

Use ThreadLocal.withInitial

You can set your LOCAL_RANDOM with the following:

private static final ThreadLocal LOCAL_RANDOM = ThreadLocal.withInitial(Random::new);


You need to give a Supplier as argument, and that is exactly what Random::new is.

Reduce logic involved in apportion

In my opinion too much is going on is this method, there needs to be a way to write it in a cleaner way. An example is the following, though it still does not eliminate all increment and decrement operations:

/**
 * Split a number of samples as evenly as possible over the number of available processors.
 * 
 * This method will for example split 77 over 4 as [20, 19, 19, 19]
 * 
 * @param samples the samples to split
 * @return an array containing the number of samples to process on each processor.
 */
private static final long[] apportion(final long samples) {
    int cores = CPU_COUNT;
    long[] portions = new long[cores];

    //calculate minimum amount of samples per core
    long minSamples = samples / cores;
    Arrays.setAll(portions, index -> minSamples);

    //calculate remaining samples after evenly dividing the minimum amount over all cores
    long remaining = samples - (minSamples * cores);

    //split the remaining samples over the cores        
    for (int coreId = 0; remaining > 0; coreId++) {
        portions[coreId]++;
        remaining--;
    }

    return portions;
}


Streamify samplePortion

This suggestion can be a matter of taste, but I believe a piece of code written in a functional programming variant is easier to understand than some calculations involving loops and more importantly it is less error-prone as you cannot possibly implement the count() method incorrectly!

/**
 * Internal sampling method that counts the number of input samples that are inside the circle too.
 * @param samples the samples to calculate
 * @return the count of samples that are inside the circle.
 */
private static final long samplePortion(final long samples) {
    final Random random = LOCAL_RANDOM.get();
    return LongStream.range(0, samples)
        .filter(sample -> isInside(random))
        .count();
}


This solution also reduces the lines of code used, hence it should in theory be easier to maintain.

Use underscores in numeric literals

For clarify you might write the following in the main method:

double calc = sampleCircleArea(100_000);


Note the 100_000 over 100000.

Code Snippets

private static final ThreadLocal<Random> LOCAL_RANDOM = ThreadLocal.withInitial(Random::new);
/**
 * Split a number of samples as evenly as possible over the number of available processors.
 * 
 * This method will for example split 77 over 4 as [20, 19, 19, 19]
 * 
 * @param samples the samples to split
 * @return an array containing the number of samples to process on each processor.
 */
private static final long[] apportion(final long samples) {
    int cores = CPU_COUNT;
    long[] portions = new long[cores];

    //calculate minimum amount of samples per core
    long minSamples = samples / cores;
    Arrays.setAll(portions, index -> minSamples);

    //calculate remaining samples after evenly dividing the minimum amount over all cores
    long remaining = samples - (minSamples * cores);

    //split the remaining samples over the cores        
    for (int coreId = 0; remaining > 0; coreId++) {
        portions[coreId]++;
        remaining--;
    }

    return portions;
}
/**
 * Internal sampling method that counts the number of input samples that are inside the circle too.
 * @param samples the samples to calculate
 * @return the count of samples that are inside the circle.
 */
private static final long samplePortion(final long samples) {
    final Random random = LOCAL_RANDOM.get();
    return LongStream.range(0, samples)
        .filter(sample -> isInside(random))
        .count();
}
double calc = sampleCircleArea(100_000);

Context

StackExchange Code Review Q#74780, answer score: 10

Revisions (0)

No revisions yet.