NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

[Form] Improve rounding precision#21769

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Closed

jonny-no1 wants to merge3 commits intosymfony:2.7fromjonny-no1:issue-21734

Closed

[Form] Improve rounding precision#21769

jonny-no1 wants to merge3 commits intosymfony:2.7fromjonny-no1:issue-21734

Conversation

Copy link

jonny-no1 commentedFeb 26, 2017•
edited
Loading

Q	A
Branch?	2.7
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	21734
License	MIT
Doc PR

For full details see#21734 from#21734 (comment) onwards.

Excerpt:

$number =floor(37.37*100);// double(3736)var_dump($number);$number =floor(bcmul(37.37,100));// double(3737)var_dump($number);

Fromhttp://php.net/manual/en/language.types.float.php#language.types.float:

Additionally, rational numbers that are exactly representable as floating point numbers in base 10, like 0.1 or 0.7, do not have an exact representation as floating point numbers in base 2, which is used internally, no matter the size of the mantissa. Hence, they cannot be converted into their internal binary counterparts without a small loss of precision. This can lead to confusing results: for example, floor((0.1+0.7)*10) will usually return 7 instead of the expected 8, since the internal representation will be something like 7.9999999999999991118....
So never trust floating number results to the last digit, and do not compare floating point numbers directly for equality. If higher precision is necessary, the arbitrary precision math functions and gmp functions are available.

carsonbot added Status: Needs Review Form Bug labels

Feb 26, 2017

Copy link

Contributor

theofidry commentedFeb 26, 2017

To apply that change it means the BCMath extension must be enabled. I don't expect to be an issue as it's shipped by default in most PHP installations, but it might be good to either makebcmath required in thecomposer.json or add a polyfill.

xabbuh added this to the2.7 milestone

Feb 26, 2017

Copy link

Member

javiereguiluz commentedFeb 26, 2017

@theofidry sadly anything that starts with"this just needs a simple PHP extension enabled..." is an issue. Requiring a whole PHP extension for this small feature is a no-go ... so we should go for a polyfill ... or enable this feature conditionally if BCmath is available?

Copy link

Author

jonny-no1 commentedFeb 26, 2017

I suggest we enable this conditionally as it's covering an edge case. Something along the lines of:

$number =function_exists('bcmul') ?bcmul($number,$roundingCoef,6) :$number *$roundingCoef;

if (function_exists('bcmul')) {$number =bcmul($number,$roundingCoef,6);}else {$number *=$roundingCoef;}

We could then isolate the test and skip it if BC Math isn't available.

Copy link

Member

nicolas-grekas commentedFeb 28, 2017

This works without bcmath:

--- a/src/Symfony/Component/Form/Extension/Core/DataTransformer/NumberToLocalizedStringTransformer.php+++ b/src/Symfony/Component/Form/Extension/Core/DataTransformer/NumberToLocalizedStringTransformer.php@@ -265,34 +265,34 @@ class NumberToLocalizedStringTransformer implements DataTransformerInterface     {         if (null !== $this->precision && null !== $this->roundingMode) {             // shift number to maintain the correct scale during rounding-            $roundingCoef = pow(10, $this->precision);+            $roundingCoef = pow(10, $this->precision + 1);             $number *= $roundingCoef;              switch ($this->roundingMode) {                 case self::ROUND_CEILING:-                    $number = ceil($number);+                    $number = ceil(ceil($number) / 10);                     break;                 case self::ROUND_FLOOR:-                    $number = floor($number);+                    $number = floor(floor($number) / 10);                     break;                 case self::ROUND_UP:-                    $number = $number > 0 ? ceil($number) : floor($number);+                    $number = $number > 0 ? ceil(ceil($number) / 10) : floor(floor($number) / 10);                     break;                 case self::ROUND_DOWN:-                    $number = $number > 0 ? floor($number) : ceil($number);+                    $number = $number > 0 ? floor(floor($number) / 10) : ceil(ceil($number) / 10);                     break;                 case self::ROUND_HALF_EVEN:-                    $number = round($number, 0, PHP_ROUND_HALF_EVEN);+                    $number = round(round($number, 0, PHP_ROUND_HALF_EVEN) / 10, 0, PHP_ROUND_HALF_EVEN);                     break;                 case self::ROUND_HALF_UP:-                    $number = round($number, 0, PHP_ROUND_HALF_UP);+                    $number = round(round($number, 0, PHP_ROUND_HALF_UP) / 10, 0, PHP_ROUND_HALF_UP);                     break;                 case self::ROUND_HALF_DOWN:-                    $number = round($number, 0, PHP_ROUND_HALF_DOWN);+                    $number = round(round($number, 0, PHP_ROUND_HALF_DOWN) / 10, 0, PHP_ROUND_HALF_DOWN);                     break;             }-            $number /= $roundingCoef;+            $number /= $roundingCoef / 10;         }          return $number;

Copy link

Contributor

dmaicher commentedFeb 28, 2017•
edited
Loading

@nicolas-grekas I don't think this solves the issue 😉 It may work for the37.37 case but due to the limited precision offloat numbers it fails for other numbers.

Example test case that fails with your code insideNumberToLocalizedStringTransformerTest::reverseTransformWithRoundingProvider:

array(2,'2.01',2.01, NumberToLocalizedStringTransformer::ROUND_DOWN),

Gives:

There was 1 failure:1) Symfony\Component\Form\Tests\Extension\Core\DataTransformer\NumberToLocalizedStringTransformerTest::testReverseTransformWithRounding with data set #33 (2, '2.01', 2.0099999999999998, 2)Failed asserting that 2.0 matches expected 2.0099999999999998.

I found this case using a little script:

<?php$i =1;while(true) {$i +=0.01;$i = (string)$i;$number =$i *1000;$floored =floor($number);var_dump($i);var_dump($number);var_dump($floored);if ((string)$floored !== (string)$number) {echo"FAIL!" .PHP_EOL;        die;    }echo'----------' .PHP_EOL;}

==>

...string(4) "2.01"float(2010)float(2009)FAIL!

I don't think this can be solved without using a calculation with "arbitrary precision" like bcmath.

Copy link

Contributor

dmaicher commentedFeb 28, 2017•
edited
Loading

Interesting is that this seems to work as well:

--- a/src/Symfony/Component/Form/Extension/Core/DataTransformer/NumberToLocalizedStringTransformer.php+++ b/src/Symfony/Component/Form/Extension/Core/DataTransformer/NumberToLocalizedStringTransformer.php@@ -242,7 +242,7 @@ class NumberToLocalizedStringTransformer implements DataTransformerInterface         if (null !== $this->scale && null !== $this->roundingMode) {             // shift number to maintain the correct scale during rounding             $roundingCoef = pow(10, $this->scale);-            $number *= $roundingCoef;+            $number = (string) ($number * $roundingCoef);              switch ($this->roundingMode) {                 case self::ROUND_CEILING:

So far I could not find a case where it fails 😋

I guess this is quite similar to thebcmul solution as it stores the result of the multiplication as a string to avoid precision issues?

Copy link

Member

nicolas-grekas commentedMar 2, 2017•
edited
Loading

@foaly-nr1 would you update you PR using@dmaicher's patch, and add the test case he's suggesting also?

Copy link

Author

jonny-no1 commentedMar 2, 2017

@nicolas-grekas on it.

dmaicher reviewed

Mar 2, 2017

View reviewed changes

...Symfony/Component/Form/Extension/Core/DataTransformer/NumberToLocalizedStringTransformer.php

		// shift number to maintain the correct scale during rounding
		$roundingCoef =pow(10,$this->precision);
		$number*=$roundingCoef;
		$number= (string) ($number *$roundingCoef);

Copy link

Contributor

dmaicherMar 2, 2017

Choose a reason for hiding this comment

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

Maybe also add a comment here why we chose the string representation?

foaly-nr1 added3 commits

March 2, 2017 11:19

Improve rounding precision

dae8f6a

String representation to avoid rounding errors, credit to@dmaicher

ceee5d5

Adds comment

6ab0784

jonny-no1 force-pushed theissue-21734 branch fromc3faf46 to6ab0784Compare

March 2, 2017 11:20

Copy link

Author

jonny-no1 commentedMar 2, 2017

Just rebased but tests are still failing as the 2.7 build is currently broken.

Copy link

Member

fabpot commentedMar 2, 2017

Thank you @foaly-nr1.

fabpot added a commit that referenced this pull request

Mar 2, 2017

bug#21769[Form] Improve rounding precision (foaly-nr1)

0581218

This PR was squashed before being merged into the 2.7 branch (closes#21769).Discussion----------[Form] Improve rounding precision| Q             | A| ------------- | ---| Branch?       | 2.7| Bug fix?      | yes| New feature?  | no| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets | 21734| License       | MIT| Doc PR        |For full details see#21734 from#21734 (comment) onwards.Excerpt:```php$number = floor(37.37*100); // double(3736)var_dump($number);$number = floor(bcmul(37.37, 100)); // double(3737)var_dump($number);```Fromhttp://php.net/manual/en/language.types.float.php#language.types.float:> Additionally, rational numbers that are exactly representable as floating point numbers in base 10, like 0.1 or 0.7, do not have an exact representation as floating point numbers in base 2, which is used internally, no matter the size of the mantissa. Hence, they cannot be converted into their internal binary counterparts without a small loss of precision. This can lead to confusing results: for example, floor((0.1+0.7)*10) will usually return 7 instead of the expected 8, since the internal representation will be something like 7.9999999999999991118....>> So never trust floating number results to the last digit, and do not compare floating point numbers directly for equality. If higher precision is necessary, the arbitrary precision math functions and gmp functions are available.Commits-------e50804c [Form] Improve rounding precision

fabpot closed this

Mar 2, 2017

Copy link

Member