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

Calculating pi by adding areas of thin rectangles

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

Problem

I wrote a small program for fun to try to prove Pi by taking a certain precision and radius and using it to calculate the area of the circle. My method should be giving me an area that is just slightly larger than the actual area of the circle, approaching the actual area as the precision gets smaller.

The funny thing is that this area should always be larger than the actual area, but when I set the precision to be less than 1.0e-9L (say 1.0e-10L), I get a result smaller than 3.141592654. Can you look at my code and tell me where I messed up if anywhere?

```
#include
#include
#include
#include
#include

using namespace std;

namespace Settings{
register constexpr long double precision = 1.0e-10L;
register constexpr long double radius = 1.0e0L;
}

// Make the math cleaner
inline long double SQUARE(register long double Val) { return Val * Val; }

// Main calculate function
long double get_Area(register long double radius = Settings::radius);

int main()
{
time_t benchmark = time(0);
register long double area = get_Area();
register long double pi = area / SQUARE(Settings::radius);

cout A1 = async(launch::async, part, 0.0L, (1.0L / 8.0L) * radius, max_Area);
future A2 = async(launch::async, part, (1.0L / 8.0L) radius, (2.0L / 8.0L) radius, max_Area);
future A3 = async(launch::async, part, (2.0L / 8.0L) radius, (3.0L / 8.0L) radius, max_Area);
future A4 = async(launch::async, part, (3.0L / 8.0L) radius, (4.0L / 8.0L) radius, max_Area);
future A5 = async(launch::async, part, (4.0L / 8.0L) radius, (5.0L / 8.0L) radius, max_Area);
future A6 = async(launch::async, part, (5.0L / 8.0L) radius, (6.0L / 8.0L) radius, max_Area);
future A7 = async(launch::async, part, (6.0L / 8.0L) radius, (7.0L / 8.0L) radius, max_Area);
future A8 = async(launch::async, part, (7.0L / 8.0L) * radius, radius, max_Area);

area = A1.get() + A2.get() + A3.get() + A4.get() + A5.get

Solution

I see a number of things that could help you improve your program.

Don't abuse using namespace std

Putting using namespace std at the top of every program is a bad habit that you'd do well to avoid. Know when to use it and when not to (as when writing include headers). In this particular case, I happen to think it's perfectly appropriate because it's a single short program that and not a header. Some people seem to think it should never be used under any circumstance, but my view is that it can be used as long as it is done responsibly and with full knowledge of the consequences.

Don't use system("pause")

There are two reasons not to use system("cls") or system("pause"). The first is that it is not portable to other operating systems which you may or may not care about now. The second is that it's a security hole, which you absolutely must care about. Specifically, if some program is defined and named cls or pause, your program will execute that program instead of what you intend, and that other program could be anything. First, isolate these into a seperate functions cls() and pause() and then modify your code to call those functions instead of system. Then rewrite the contents of those functions to do what you want using C++. For example:

void pause() {
    getchar();
}


Don't use register

As of C++11, the use of the register keyword is deprecated. In C++17, it will be gone, so it's wise to avoid its use.

Be careful with floating point

Adding a small number to a large number using floating point is risky because in certain cases, as potentially with your for loop within get_Area(). Specifically, with floating point numbers, adding a large number to a small number can lead to loss of precision. In extreme cases, if a is very large and b is very small, we can have a + b = a which is obviously incorrect for non-zero values of b. (See the oft-cited and still excellent What Every Computer Scientist Should Know About Floating-Point Arithmetic, by David Goldberg for a readable technical discussion for why this is so.) I should mention that it will not happen with this particular code with these particular values, but it's a potential problem that we programmers should all understand.

Consider using portable scalability

The choice of 8 parts is just fine, but you might consider using std::thread::hardware_concurrency for a portable way to scale on multiprocessor systems.

Don't repeat yourself

Rather than calculating the low and high values for each slice manually, and naming each std::future by hand, have the computer do it instead:

const unsigned slices = thread::hardware_concurrency();
vector> A(slices);
const long double increment = radius / slices;
long double lo = 0;
long double hi = increment;
for (unsigned i=0; i &f){ 
        return accum + f.get(); 
    }) * 4;


Extend the accuracy

Since you're using a simple Riemann sum, on the quarter circle you've chosen the sum will indeed always be greater than the actual value. However, we can very simply take advantage of the symmetry of the problem and have that error mostly cancel out by summing the entire top half of the circle. That is, instead of $$\pi = 4\int_0^{r} \sqrt{r^2 - x^2}\delta x$$ we can instead compute $$\pi = 2\int_{-r}^{r} \sqrt{r^2 - x^2}\delta x$$.

Omit return 0

When a C++ program reaches the end of main the compiler will automatically generate code to return 0, so there is no reason to put return 0; explicitly at the end of main.

Code Snippets

void pause() {
    getchar();
}
const unsigned slices = thread::hardware_concurrency();
vector<future<long double>> A(slices);
const long double increment = radius / slices;
long double lo = 0;
long double hi = increment;
for (unsigned i=0; i<slices; ++i) {
    A[i] =  async(launch::async, part, lo, hi, max_Area);
    lo += increment;
    hi += increment;
}
return std::accumulate(A.begin(), A.end(), 0.0L, 
    [](long double accum, std::future<long double> &f){ 
        return accum + f.get(); 
    }) * 4;

Context

StackExchange Code Review Q#126570, answer score: 6

Revisions (0)

No revisions yet.