Added intergration of Monte Carlo for PHP #431

PudottaPommin · 2018-10-03T14:27:44Z

No description provided.

Gorzoid

Code runs fine for radius = 1, although it takes quite a while to run with 100 million samples.

Gorzoid · 2018-10-03T21:44:53Z

contents/monte_carlo_integration/code/php/monte_carlo.php

+$percentError = abs($piEstimate - pi()) / pi() * 100;
+echo sprintf('The estimate of PI is: %s', $piEstimate);
+echo PHP_EOL;
+echo sprintf('The percent error is: %s', $percentError);


Missing ?> at the end, also should have 1 newline at end of file.

?> is discouraged and not needed since PHP 5.3 ( I think that version, not sure if 2/3/4 ).
I haven't left newline in any of previous PHP files, so this is with same style and format

The comment about ?> was a mixup where I thought the code wasn't running because it didn't read a closing tag. It actually was running but was very slow.
The newline requirement is a git requirement rather than a php requirement iirc, Github warns that there is no newline at the end of file. I think it's because it messes with deltas of files.

Gorzoid · 2018-10-03T21:48:49Z

contents/monte_carlo_integration/code/php/monte_carlo.php

+
+function in_circle(float $positionX, float $positionY, float $radius = 1): bool
+{
+  return pow($positionX, 2) + pow($positionY, 2) < pow(abs($radius), 2);


I think you should just do
$positionX * $positionX + $positionY * $positionY + $positionZ * $positionZ
The abs also seems completley unnecessary since x^2 is always positive.

This still hasn't been resolved btw.

Gorzoid · 2018-10-03T21:56:14Z

contents/monte_carlo_integration/code/php/monte_carlo.php

+
+function random_zero_to_one(): float
+{
+  return mt_rand(0, 100) / 100;


Is there a reason for the 100? The recomended way to get a random float is the same as C:
return mt_rand() / mt_getrandmax();

Yes, there is. We want number in interval <0,1> so I'm getting int in range <0,100> and dividing 100 to get required number

That results in only 101 posible values 0.000, 0.001, 0.002, ..., 0.999, 1.0, running over 101^2 times would then be meaningless, instead you should take advantage of the full range of floating point precision by dividing by mt_getrandmax()

I see, it's changed, thanks for explanation :)

Gorzoid · 2018-10-03T22:18:15Z

contents/monte_carlo_integration/code/php/monte_carlo.php

+  $in_circle_count = 0;
+
+  for ($i = 0; $i < $samples; $i++) {
+    if (in_circle(random_zero_to_one(), random_zero_to_one(), $radius))


x and y position need to be multiplied by $radius for proper range of random values.

Haven't seen any of the other implementations use radius to multiply X or Y. So what's different in this?

Other implementations chose for radius to be a constant 1.0, you put radius as a function parameter which default value 1.0
Either you can make radius constant 1.0 or implement it so it correctly calculates pi when the radius parameter is not equal to 1.0
Take a look at the python implementation, they generate random numbers from 0 to radius. The easiest way to do this is just multiply your existing random function by $radius.

Gorzoid · 2018-10-03T22:22:41Z

contents/monte_carlo_integration/code/php/monte_carlo.php

+
+$piEstimate = monte_carlo(100000000);
+$percentError = abs($piEstimate - pi()) / pi() * 100;
+echo sprintf('The estimate of PI is: %s', $piEstimate);


PHP has a printf function

Probably should use %f for these values, php converts em anyway but no harm in being more correct.

Why are we using PHP_EOL rather than \n, it doesn't work for single quote strings but they work fine in double quotes
printf("The estimate of PI is: %f\n", $piEstimate);

it has, but there is not much difference and sprintf gives easier way to extract variable

%f would generate X.XXXXX format when %s givex X.XX (depending on number of numbers after dot)

PHP_EOL is constant depending on environment. from SO "PHP_EOL represents the endline character for the current system", so it's much prefered to use it

Fair enough, I don't understand what you mean by "sprintf gives easier way to extract variable" but your other arguments make sense, I was under the impression PHP would \n to the correct line ending like C/C++.

it's basically from my experience, where you don't print to console, but you need to work with strings, build them, etc. You can store sprintf to variable and work with it later. printf would print to output.

yes, it would but with PHP_EOL. otherwise if you specify \n it uses that, which might be problem on some platforms.

You can store sprintf to variable and work with it later.

You do not do that, though, so there's no reason to use it. I have to agree with @Gorzoid here. printf() makes a little more sense.

I give you this one, I'm gonna change it. But still I'm staying, that sprintf looks better 🤣

berquist · 2018-10-04T01:21:39Z

For the newline at the end of files, if you have an editor or IDE that supports EditorConfig, it can be added automatically, along with enforcing other style guidelines:

# PHP
[*.php]
indent_style = space
indent_size = 4

which I'm having a hard time understanding (https://stackoverflow.com/questions/1584481/php-line-indentation), so I don't know what they should be.

PudottaPommin · 2018-10-04T01:30:05Z

@berquist I know it can be set, but there is not any setting for PHP in editorconfig.
I personally very much dislike 4 spaces indent (or 1 tab wide 4 spaces). It just takes too much space in a bit deeper levels/conditions...

About the newline. What's the benefit of that in PHP file?

berquist · 2018-10-04T01:40:34Z

I was sort of prompting you to add the correct settings to the .editorconfig file, because I don't know what they should be, and you probably do :)

See here for why adding a newline may be considered important: https://stackoverflow.com/q/5813311 (again, EditorConfig will do this automatically when the plugin is on).

PudottaPommin · 2018-10-04T09:39:30Z

1st merge #442 , then I modify code style :)

Changed random_zero_to_one float calculation

Multiplying X,Y by radius

Butt4cak3

I don't have many complaints and they are exclusively related to code style, but they're somewhat important and we should talk about them before we continue adding PHP code to the project.

contents/monte_carlo_integration/code/php/monte_carlo.php

changed sprintf to printf

changed function parameters to camel_case

PudottaPommin · 2018-10-06T17:53:47Z

@Butt4cak3 so my proposition:

PSR-2 standard
global functions (set outside of classes) will be snake_case
variables will be camelCase

if yes, I write it to the editorconfig PR and modify other PR based on our resulted styled.

Butt4cak3 · 2018-10-06T18:31:08Z

Agreed.

variable names changed back to camelCase

change $in_circle_count to $inCircleCount

PudottaPommin · 2018-10-07T09:49:47Z

as we talked, this should be ready for final review @Butt4cak3

Butt4cak3

Found 2 more things. Don't forget to update the line numbers in the .md file once you added the brackets for the if statement.

Butt4cak3 · 2018-10-07T13:03:30Z

contents/monte_carlo_integration/code/php/monte_carlo.php

+    $inCircleCount = 0;
+
+    for ($i = 0; $i < $samples; $i++) {
+        if (in_circle(random_zero_to_one() * $radius, random_zero_to_one() * $radius, $radius))


Taken from PSR-2:

The body of each structure MUST be enclosed by braces. This standardizes how
the structures look, and reduces the likelihood of introducing errors as new
lines get added to the body.

Butt4cak3 · 2018-10-07T13:05:22Z

contents/monte_carlo_integration/code/php/monte_carlo.php

+
+function in_circle(float $positionX, float $positionY, float $radius = 1): bool
+{
+    return pow($positionX 2) + pow($positionY, 2) < pow($radius, 2);


Missing comma between $positionX and 2.

Added intergration of Monte Carlo for PHP

4bc5868

june128 added Implementation This provides an implementation for an algorithm. (Code and maybe md files are edited.) Hacktoberfest The label for all Hacktoberfest related things! labels Oct 3, 2018

Gorzoid suggested changes Oct 3, 2018

View reviewed changes

PudottaPommin added 3 commits October 4, 2018 11:22

Changed code style

b411169

Update monte_carlo.php

559c7fc

Changed random_zero_to_one float calculation

Update monte_carlo.php

95eafaf

Multiplying X,Y by radius

Butt4cak3 requested changes Oct 4, 2018

View reviewed changes

contents/monte_carlo_integration/code/php/monte_carlo.php Show resolved Hide resolved

contents/monte_carlo_integration/code/php/monte_carlo.php Show resolved Hide resolved

contents/monte_carlo_integration/code/php/monte_carlo.php Show resolved Hide resolved

Butt4cak3 self-assigned this Oct 4, 2018

Butt4cak3 mentioned this pull request Oct 4, 2018

Implementation Stable marriage problem in PHP #428

Merged

PudottaPommin added 2 commits October 5, 2018 13:19

Update monte_carlo.php

a7a3738

changed sprintf to printf

Update monte_carlo.php

afa9d6d

changed function parameters to camel_case

PudottaPommin added 3 commits October 6, 2018 23:04

Update monte_carlo.php

ed52163

variable names changed back to camelCase

Update monte_carlo.php

ec2f420

change $in_circle_count to $inCircleCount

Merge branch 'master' into php_monte_carlo

e3409c6

Butt4cak3 requested changes Oct 7, 2018

View reviewed changes

Styles fixes and typo

4a241aa

Butt4cak3 approved these changes Oct 7, 2018

View reviewed changes

Butt4cak3 merged commit c6ff7bb into algorithm-archivists:master Oct 7, 2018

PudottaPommin deleted the php_monte_carlo branch October 7, 2018 15:01

Uh oh!

Added intergration of Monte Carlo for PHP #431

Added intergration of Monte Carlo for PHP #431

Uh oh!

Conversation

PudottaPommin commented Oct 3, 2018

Uh oh!

Gorzoid left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Butt4cak3 Oct 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

berquist commented Oct 4, 2018

Uh oh!

PudottaPommin commented Oct 4, 2018

Uh oh!

berquist commented Oct 4, 2018

Uh oh!

PudottaPommin commented Oct 4, 2018

Uh oh!

Butt4cak3 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

PudottaPommin commented Oct 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Butt4cak3 commented Oct 6, 2018

Uh oh!

PudottaPommin commented Oct 7, 2018

Uh oh!

Butt4cak3 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Butt4cak3 Oct 4, 2018 •

edited

Loading

PudottaPommin commented Oct 6, 2018 •

edited

Loading