Calculating the value of pi-what is wrong with my code

Question

I'm doing another C++ exercise. I have to calculate the value of pi from the infinite series:

pi=4 - 4/3 + 4/5 – 4/7 + 4/9 -4/11+ . . .

The program has to print the approximate value of pi after each of the first 1,000 terms of this series. Here is my code:

#include <iostream> using namespace std; int main() { double pi=0.0; int counter=1; for (int i=1;;i+=2)//infinite loop, should "break" when pi=3.14159 { double a=4.0; double b=0.0; b=a/static_cast<double>(i); if(counter%2==0) pi-=b; else pi+=b; if(i%1000==0)//should print pi value after 1000 terms,but it doesn't cout<<pi<<endl; if(pi==3.14159)//this if statement doesn't work as well break; counter++; } return 0; }

It compiles without errors and warnings, but only the empty console window appears after execution. If I remove line” if(i%1000==0)” , I can see it does run and print every pi value, but it doesn’t stop, which means the second if statement doesn’t work either. I’m not sure what else to do. I’m assuming it is probably a simple logical error.

Programming with Floating-Point Rule #2: never compare non-whole floating-point values for equality. — RBarryYoung
– RBarryYoung, Commented Sep 25, 2009 at 20:47

Adam Wright · Accepted Answer · 2009-09-25 20:11:40Z

20

Well, i % 1000 will never = 0, as your counter runs from i = 1, then in increments of 2. Hence, i is always odd, and will never be a multiple of 1000.

The reason it never terminates is that the algorithm doesn't converge to exactly 3.14157 - it'll be a higher precision either under or over approximation. You want to say "When within a given delta of 3.14157", so write

if (fabs(pi - 3.14157) < 0.001) break

or something similar, for however "close" you want to get before you stop.

edited Sep 25, 2009 at 20:11

answered Sep 25, 2009 at 20:01

Adam Wright

49.5k12 gold badges134 silver badges153 bronze badges

Sign up to request clarification or add additional context in comments.

9 Comments

jprete Over a year ago

I agree with almost all of your answer. HOWEVER, you should change that line of code to "if (fabs(pi - 3.14157) < 0.001)". Otherwise you are checking the wrong condition!

Adam Wright Over a year ago

Thanks for the comment, jprete - Stupid mistake, and I've changed the example :)

Mike55 Over a year ago

I have added if (fabs(pi - 3.14157) < 0.001) break and it still doesn't work. I included #<cmath> but only an empty console appears again.

Adam Wright Over a year ago

If you print the value at each iteration (i.e. remove the if), does it actually converge to a value such that |pi - 3.14157| is less than your delta?

David Thornley Over a year ago

Nitpick: I assume you meant 3.14159, not 3.14157.

|

Sterno · Accepted Answer · 2009-09-25 20:06:14Z

7

Since you start i at 1 and increment by 2, i is always an odd number, so i % 1000 will never be 0.

edited Sep 25, 2009 at 20:06

answered Sep 25, 2009 at 20:00

Sterno

1,6582 gold badges17 silver badges29 bronze badges

Comments

Moshe Levi · Accepted Answer · 2009-09-25 20:08:43Z

you have more than one problem:

A. i%1000==0 will never be true because you're iterating only odd numbers.

B. pi==3.14159 : you cannot compare double values just like that because the way floating point numbers are represented (you can read about it here in another question). in order for it to work you should compare the values in another way - one way is to subtract them from each other and check that the absolute result is lower than 0.0000001.

@Pat: The best way to say "Thanks" here is by upvoting the answer.

chaos · Accepted Answer · 2009-09-25 20:01:45Z

You have floating point precision issues. Try if(abs(pi - 3.14159) < 0.000005).
i%1000 will never be 0 because i is always odd.

JDunkerley · Accepted Answer · 2009-09-25 20:02:52Z

2

Shouldn't it be:

if (counter%1000==0)

answered Sep 25, 2009 at 20:02

JDunkerley

12.5k5 gold badges43 silver badges46 bronze badges

1 Comment

JDunkerley Over a year ago

but that would cause it to stop every 500 steps, not every 1000

Jason Berkan · Accepted Answer · 2009-09-25 20:35:21Z

i starts at 1 and then increments by 2. Therefore i is always odd and will never be a multiple of 1000, which is why if (i % 1000 == 0) never passes.
Directly comparing floats doesn't work, due to floating precision issues. You will need to compare that the difference between the values is close enough.

Phil H · Accepted Answer · 2009-09-27 17:04:53Z

pi=4 - 4/3 + 4/5 – 4/7 + 4/9 -4/11 + ...

Generalising

pi = Σ_i=0^∞ (-1)ⁱ 4 / (2i+1)

Which gives us a cleaner approach to each term; the i'th term is given by:

double term = pow(-1,i%2) * 4 / (2*i+1);

where i=0,1,2,...,N

So, our loop can be fairly simple, given some number of iterations N

int N=2000; double pi=0; for(int i=0; i<N; i++) { double term = pow(-1,i%2) * 4 / (2*(double)i+1); pi += term; cout << i << "\t" << pi <<endl; }

Your original question stated "The program has to print the approximate value of pi after each of the first 1,000 terms of this series". This does not imply any need to check whether 3.14159 has been reached, so I have not included this here. The pow(-1,i%2) call is just to avoid if statements (which are slow) and prevent any complications with large i.

Be aware that after a number of iterations, the difference between the magnitude of pi and the magnitude of the correcting term (say -4/25) will be so small that it will go beyond the precision of a double, so you would need higher precision types to deal with it.

Todd · Accepted Answer · 2009-09-25 23:50:18Z

By default abs uses the abs macro which is for int. For doubles, use the cmath library.

#include <iostream> #include <cmath> int main() { double pi=0.0; double a=4.0; int i = 1; for (i=1;;i+=2) { pi += (1 - 2 * ((i/2)%2)) * a/static_cast<double>(i); if( std::abs(pi - 3.14159) < 0.000001 ) break; if (i > 2000) //1k iterations break; } std::cout<<pi<<std::endl; return 0; }

I think the statement if (i>2000) break; is incorrect, because it breaks the loop before it reaches 3.14159 (which is more than 2000 iterations). Adding counter variable with if statement if (counter%1000==0) should fix it. I will post the corrected code.
You're correct that the i > 2k breaks before 3.14159 and doesn't really belong. In his original implementation he had two checks, neither of which worked. So, I left it in as an example.

Mike55 · Accepted Answer · 2009-09-27 13:08:06Z

Here is the corrected code. I thought it may be helpful in the future if somebody has similar problem.

#include <iostream> #include <cmath> using namespace std; int main() { double pi=0.0; int counter=1; for (int i=1;;i+=2) { double a=4.0; double b=0.0; b=a/static_cast<double>(i); if(counter%2==0) pi-=b; else pi+=b; if(counter%1000==0) cout<<pi<<" "<<counter<<endl; if (fabs(pi - 3.14159) < 0.000001) break; counter++; } cout<<pi; return 0; }

fvrghl · Accepted Answer · 2013-09-28 22:39:06Z

Here is a better one:

class pi_1000 { public: double doLeibniz( int i ) // Leibniz famous formula for pi, source: Calculus II :) { return ( ( pow( -1, i ) ) * 4 ) / ( ( 2 * i ) + 1 ); } void piCalc() { double pi = 4; int i; cout << "\npi calculated each iteration from 1 to 1000\n"; //wording was a bit confusing. //I wasn't sure which one is the right one: 0-1000 or each 1000th. for( i = 1; i < 1000; i++ ) { pi = pi + doLeibniz( i ); cout << fixed << setprecision( 5 ) << pi << "\t" << i + 1 << "\n"; } pi = 4; cout << "\npi calculated each 1000th iteration from 1 to 20000\n"; for( i = 1; i < 21000; i++ ) { pi = pi + doLeibniz( i ); if( ( ( i - 1 ) % 1000 ) == 0 ) cout << fixed << setprecision( 5 ) << pi << "\t" << i - 1 << "\n"; } }

Collectives™ on Stack Overflow

Calculating the value of pi-what is wrong with my code

10 Answers 10

9 Comments

Comments

1 Comment

Comments

1 Comment

Comments

Comments

2 Comments

Comments

Comments

Hot Network Questions

Collectives™ on Stack Overflow

10 Answers 10

9 Comments

Comments

1 Comment

Comments

1 Comment

Comments

Comments

2 Comments

Comments

Comments

Related