Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add Python code to Monte Carlo Integration #294

Closed
wants to merge 2 commits into from
Closed

Add Python code to Monte Carlo Integration #294

wants to merge 2 commits into from

Conversation

spadaval
Copy link

No description provided.

Butt4cak3
Butt4cak3 requested changes Jul 23, 2018
View reviewed changes
Copy link
Contributor

@Butt4cak3 Butt4cak3 left a comment

Choose a reason for hiding this comment

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

Hello and welcome to the project! Thank you very much for the contribution.

I can't review the entire thing right now, but there is one thing you will definitely have to change.

contents/monte_carlo_integration/code/python/monte_carlo.py Outdated
from math import pi


def inCircle(xPos, yPos):
Copy link
Contributor

Choose a reason for hiding this comment

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

Functions and variables are usually named using snake_case rather than camelCase in Python. For example,. this line should rather be:

def in_circle(x_pos, y_pos):

This goes for everything in this file.

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

I totally forgot this, but we currently have multiple Python Monte Carlo pull requests. I can't guarantee that your code will actually be used because other people were faster than you. I'll ask @jiegillet about the situation as soon as he's back since he's handling the situation as far as I know.

@jiegillet jiegillet self-assigned this Jul 24, 2018
@jiegillet
Copy link
Member

Hi @hybrideagle, thanks for you contribution. #184 is ahead of you. Feel free to look at my review of that code and change your code accordingly if you wish, but I will only accept your code if that other PR goes dead. That has happened 2 or 3 times before for monte carlo in Python, somehow this combination is cursed.

@spadaval
Copy link
Author

This is the weirdest algorithm to have this kind of churn.
Should I close this PR for now?

@jiegillet jiegillet added the Duplicate This is a duplicate of another pull request or issue. label Jul 24, 2018
@jiegillet
Copy link
Member

I think you can close it. We are close to resolution with the other PR.

@spadaval spadaval closed this Jul 25, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Butt4cak3 Butt4cak3 Butt4cak3 requested changes

Assignees

@jiegillet jiegillet

Labels
Duplicate This is a duplicate of another pull request or issue. Implementation This provides an implementation for an algorithm. (Code and maybe md files are edited.)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@spadaval @Butt4cak3 @jiegillet @Gathros