Added python3 to Monte Carlo Integration #144
Conversation
jiegillet
added
the
Implementation
|
|
||
| #estimate pi by integrating the unit circle | ||
| estimated_area = monte_carlo(10**6, 1) | ||
| print("Estimation for pi: %.10f"%estimated_area) |
There was a problem hiding this comment.
Isn't it preferred to use string interpolation? "Estimation for pi: {}".format(estimated_area)
There was a problem hiding this comment.
Even better, we could use format strings: f"Estimation for pi: {esimated_area}". I don't remember how to specify precision off the top of my head but it shouldn't be that difficult.
There was a problem hiding this comment.
print( "Estimation for pi: {:.10f}".format(estimated_area) )
| for i in range(n): | ||
| count += in_circle(random.uniform(0,radius),random.uniform(0,radius),radius) | ||
| estimated_area = 4*(count/n)*radius**2 | ||
| exact_area = math.pi*radius**2 |
There was a problem hiding this comment.
I would do the solution comparison outside of monte_carlo, it's not part of the algorithm.
|
I believe the code is incorrect. If you pick your random numbers between 0 and radius, you don't need to renormalize by the radius later on, you only need to do so if you pick between 0 and 1. |
|
If the only goal is to approximate pi, multiplying with radius squared is indeed unnecessary. My thought was that the example for monte carlo integration should contain a function to determine the area of the circle, rather than just pi. Also the use of the radius parameter is to stay consistent with the Julia code already present. |
|
I prefer the radius to be in the code (but it ultimately doesn't matter). Having the the |
|
@jonasvandervennet No, you misunderstand, beyond the question of using It's a terrible approximation ^^ You can fix it by either replacing My suggestion: keep the parameter |
|
hey @jonasvandervennet I apologize for this. The initial julia code was confusing. I had a discussion with @jiegillet and he convinced me to re-write the code without the If it is easier to use that code as a base, please feel free to do that. Sorry again for the confusion! |
|
I've submitted a pull request to @jonasvandervennet fork, mainly fixing readability issues. |
| @@ -41,6 +41,8 @@ each point is tested to see whether it's in the circle or not: | |||
| [import:2-8, lang:"julia"](code/julia/monte_carlo.jl) | |||
| {% sample lang="hs" %} | |||
| [import:7-7, lang:"haskell"](code/haskell/monteCarlo.hs) | |||
| {% sample lang="py3" %} | |||
There was a problem hiding this comment.
We just changed some things and settled on py instead of py3, basically just requiring code to be submitted in python3. Would you be able to change this to py?
| @@ -78,6 +80,9 @@ Feel free to submit your version via pull request, and thanks for reading! | |||
| {% sample lang="hs" %} | |||
| ### Haskell | |||
| [import, lang:"haskell"](code/haskell/monteCarlo.hs) | |||
| {% sample lang="py3" %} | |||
| import random, math | ||
|
|
||
| # function to determine whether an x, y point is inside a circle with given radius | ||
| def in_circle(x, y, radius): |
There was a problem hiding this comment.
We slightly modified most implementations to hide the radius variable because it was confusing some people and we specified that we were working on the unit circle. Would you be able to remove the radius variable?
| return x**2 + y**2 < radius**2 | ||
|
|
||
| # function to integrate a circle with given radius via monte carlo integration | ||
| def monte_carlo(n, radius): |
|
Actually, I just checked the PR from @GuyPozner . It seems to make all the necessary updates. If you merge that PR into your own fork @jonasvandervennet , we should be mostly good to go here. |
|
Dear @jonasvandervennet, you've submitted your PR about 8 days ago, but haven't responded to our reviews for 7 days. In the mean time 3 new PRs were submitted for Monte Carlo in Python, we've turned them all down, even though they were closer to a finished product than your PR. I even believe on PR was submitted to your branch. |
|
Time's up. |
No description provided.