Skip to content

Python3 monte carlo #148

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from
Closed

Python3 monte carlo #148

wants to merge 2 commits into from

Conversation

RonPeleg
Copy link

clean code for monte carlo algo

@Butt4cak3 Butt4cak3 added the Implementation This provides an implementation for an algorithm. (Code and maybe md files are edited.) label Jun 28, 2018
@Butt4cak3
Copy link
Contributor

Butt4cak3 commented Jun 28, 2018

I'm afraid you're a bit late. People are already working on Monte Carlo in Python over in #144. If you want, you can join the conversation and suggest some changes. Sorry for that!

We weren't really prepared for the sudden rush of contributors. We're of course still thankful for your contribution. The timing was just a bit unlucky. Feel free to add a code example for another algorithm and check that there's not an open pull request dealing with it already.

@Butt4cak3 Butt4cak3 self-assigned this Jun 29, 2018
@Butt4cak3 Butt4cak3 closed this Jun 30, 2018
@june128 june128 added Duplicate This is a duplicate of another pull request or issue. and removed Implementation This provides an implementation for an algorithm. (Code and maybe md files are edited.) labels Jun 30, 2018
@jiegillet
Copy link
Member

Hi @RonPeleg, the person that sent the PR that came right before you is unresponsive, so we are switching to you. Are you up for it? I only have a few points for you to change.

@jiegillet jiegillet reopened this Jul 7, 2018
@jiegillet
Copy link
Member

One more thing: you need to include your code into chapters/monte_carlo/monte_carlo.md. You can mimic how the other languages do it.

@@ -5,3 +5,4 @@ Gathros
Jeremie Gillet (- Jie -)
Salim Khatib
Hitesh C
name
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's probably better if you put in your actual name :)

return x ** 2 + y ** 2 < r ** 2


def monte_carlo(n, r):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have moved away from using r as a free parameter in monte_carlo, so you can remove it here, and call in_circle(x, y, 1) instead of in_circle(x, y, r). Another, clean alternative is to change def in_circle(x, y, r): to def in_circle(x, y, r = 1): and then just use it as in_circle(x, y).

if in_circle(x, y, r):
points_in_circle += 1

pi_estimate = 4.0 * points_in_circle / (n * r ** 2)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then in this line you don't need r ** 2 anymore.

pi_erro = 100*(math.pi - pi_estimate)/math.pi
return pi_estimate, pi_erro

print(monte_carlo(10000000, 0.5))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The last thing is, it's best if you have a main() function that prints something like "The estimated value of pi is..." and "the Error is...". The error should be calculated in main() as well because it's not part of the algorithm.

@jiegillet jiegillet added Implementation This provides an implementation for an algorithm. (Code and maybe md files are edited.) and removed Duplicate This is a duplicate of another pull request or issue. labels Jul 7, 2018
@Butt4cak3 Butt4cak3 assigned jiegillet and unassigned Butt4cak3 Jul 12, 2018
@Butt4cak3
Copy link
Contributor

Actually, you know what? I have this suspicion that @RonPeleg is already gone. So let's give them another day (it has been 5 at this time) and then move on if we get no answer.

@jiegillet
Copy link
Member

OK. We have other PRs for the same algorithm.

@Butt4cak3
Copy link
Contributor

I'd honestly just take the branch of the inactive PR that everyone was working on and create a new PR based on that.

@jiegillet jiegillet closed this Jul 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Implementation This provides an implementation for an algorithm. (Code and maybe md files are edited.)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants