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

Py monte carlo #184

Closed

GuyPozner wants to merge 4 commits into algorithm-archivists:master from GuyPozner:py-monte-carlo

Contributor

GuyPozner commented Jun 30, 2018

I've review the current pull request for the python implementation and made change to resemble the current c and julia versions. Hope that it helps.

Contributor

Gathros commented Jun 30, 2018

Thanks for your pull request, I like your version then #144 but you need to talk to jonasvandervennet on which python code to use. If you want to improve his python make a pull request to his fork.

Gathros added the Duplicate label

Contributor

Butt4cak3 commented Jun 30, 2018

Hello and welcome, @GuyPozner! Thanks for contributing to the project! We always appreciate it when people spend their time to improve the Algorithm Archive.

Unfortunately, we can't just take this pull request. As you know, people are already working on Monte Caro in Python over in #144. If you want to help with it or if you feel like something is wrong with it, you should join the discussion of the other pull request instead of creating a new one. You could even contribute code to the other PR like @Gathros already mentioned.

I see that this is your first ever pull request, so I assume you're not very familiar with the Git and GitHub workflows. Don't worry, it's alright. It's not always very intuitive and you will eventually get used to it. Feel free to ask members of our community whenever you have questions about Git or GitHub and we'll be happy to help you out!

Contributor Author

GuyPozner commented Jun 30, 2018

I understand, so if I want to fix the versions which is currently in review, I have to either wait for the new one to be summited or send a pull request of that branch to the contributor of the older version?

Contributor

Gathros commented Jun 30, 2018

Yeah, you can pick either but I recommend going send the pull request to their branch.

Member

june128 commented Jun 30, 2018

@GuyPozner That's about right I guess.
If there is currently no algorithm implementation in the AAA, but a Pull Request, that adds the algorithm implementation, you can give your input in the Pull Request and you're also able to submit your own PR to that very branch, which the PR author want to get merged into the AAA master (I highly suggest writing that in the AAA-PR as well, so that the author knows you're PRing something).
If there is currently an implementation in the AAA, you can just create a Pull Request with changes, that improve the current implementation.

For this situation, I recommend the first version, since it applies here.

Member

june128 commented Jun 30, 2018

Also don't mind just pointing out things, you think, the author of a PR should change. You don't need to make a PR for that, the author usually changes things themselves, after getting input on what to do better. (If you have any questions on how to use the GitHub interface, feel free to ask.)

Member

june128 commented Jul 1, 2018

I'm closing this PR now, if you have any questions, feel free to ask and/or join our Discord: https://discord.gg/Pr2E9S6

june128 closed this

jiegillet reopened this

Member

jiegillet commented Jul 17, 2018

Previous PRs fell through. @GuyPozner, wanna take over?

jiegillet reviewed

View reviewed changes

Member

jiegillet left a comment

A few quick changes. Also, the structure of the AAA had changed, so make sure you adapt to the current one.

chapters/monte_carlo/code/python/monte_carlo.py Outdated

		import random


		def in_circle(x, y):

Member

jiegillet Jul 17, 2018

Make it in_circle(x, y, radius = 1) it's cleaner

chapters/monte_carlo/code/python/monte_carlo.py Outdated

+              		y = random.uniform(0,1)
+              		# Count the number of points inside the circle
+              		if(in_circle(x, y, radius)):

Member

jiegillet Jul 17, 2018

radius should be dropped here.

chapters/monte_carlo/code/python/monte_carlo.py Outdated

+              	# Since we've generated points on in upper left quadrant ([0,1], [0,1])
+              	# We need to mulply the number of points by 4
+              	pi_estimate = 4 * in_circle_count / (n_samples * radius * radius)

Member

jiegillet Jul 17, 2018

Drop radius here too.

chapters/monte_carlo/code/python/monte_carlo.py Outdated

+              if __name__ == '__main__':
+              	pi_estimate = monte_carlo(100000)
+              	percent_error = abs(math.pi - pi_estimate)/math.pi

Member

jiegillet Jul 17, 2018

You need to multiply by 100 to get percents.

chapters/monte_carlo/code/python/monte_carlo.py Outdated

+              	pi_estimate = monte_carlo(100000)
+              	percent_error = abs(math.pi - pi_estimate)/math.pi
+              	print("The estimate of pi is: %.3f" % pi_estimate)

Member

jiegillet Jul 17, 2018

I would use string interpolations "The estimate of pi is: {:.3f}".format(pi_estimate). Same for next line

chapters/monte_carlo/monte_carlo.md Outdated

               {%sample lang="go" %}
               ### Go
               [import, lang:"golang"](code/go/monteCarlo.go)
+              ### Python

Member

jiegillet Jul 17, 2018

We decided to get rid of the ### Python things.

Contributor Author

GuyPozner Jul 18, 2018

I'll commit the fixed version in 24 hours.

Member

jiegillet Jul 18, 2018

Sure, take your time!
Also, please be careful that the structure of the AAA has changed since then. If you don't sync you branch first, there will be many many broken things :)

Member

jiegillet Jul 24, 2018

Any chance you could commit the fixed version? We have received yet another PR for monte carlo in python, we really need to get this one done.

Member

jiegillet commented Jul 17, 2018

Ah, somehow I was reviewing an older version. Oh well, the comments that still appear are still valid.

jiegillet mentioned this pull request

Add Python code to Monte Carlo Integration #294

Closed

GuyPozner added 3 commits

July 24, 2018 10:48


          garbage

9c574db


          garbage

9b4485e


          master sync

6f9aec3

GuyPozner force-pushed the py-monte-carlo branch from ea86ca2 to 935ae05 Compare

July 24, 2018 08:24

Contributor Author

GuyPozner commented Jul 24, 2018

Fixed.

jiegillet reviewed

View reviewed changes

Member

jiegillet left a comment

Thanks, the code is good.

However, you included change to 35 extra images files in your last commits, so I can't merge it like this. Can you fix it? Worst case scenario if it's too messy, open a new PR.

Also, please use descriptive commits. "garbage" and "garbage" simply won't do.

GuyPozner force-pushed the py-monte-carlo branch from 935ae05 to e08a735 Compare

July 24, 2018 09:37


          trying to fix binary files issue

a80cada

GuyPozner force-pushed the py-monte-carlo branch from e08a735 to a80cada Compare

July 24, 2018 09:55

Member

jiegillet commented Jul 26, 2018

Again, it might be worth closing this one and opening a new PR.

jiegillet mentioned this pull request

Adding Monte Carlo in Python, code by GuyPozner #305

Merged

Member

jiegillet commented Jul 27, 2018

Hi @GuyPozner, I'm very sorry but I bypassed this PR, because we received yet another PR for Monte Carlo earlier today. Your code has been merged, byte for byte (actually, I added two byte in the .md files because there were two commas missing).
Closing this now, thanks for the contribution ^^

jiegillet closed this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels