[Form] Improve rounding precision #21769

jonny-no1 · 2017-02-26T13:31:04Z

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);

From http://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.

theofidry · 2017-02-26T13:44:00Z

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 make bcmath required in the composer.json or add a polyfill.

javiereguiluz · 2017-02-26T19:47:49Z

@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?

jonny-no1 · 2017-02-26T19:54:04Z

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;

or

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.

nicolas-grekas · 2017-02-28T13:46:07Z

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;

dmaicher · 2017-02-28T18:02:59Z

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

Example test case that fails with your code inside NumberToLocalizedStringTransformerTest::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.

dmaicher · 2017-02-28T19:16:39Z

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 the bcmul solution as it stores the result of the multiplication as a string to avoid precision issues?

nicolas-grekas · 2017-03-02T10:47:32Z

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

jonny-no1 · 2017-03-02T10:53:17Z

@nicolas-grekas on it.

dmaicher · 2017-03-02T11:03:22Z

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

@@ -266,7 +266,7 @@ private function round($number)
        if (null !== $this->precision && null !== $this->roundingMode) {
            // shift number to maintain the correct scale during rounding
            $roundingCoef = pow(10, $this->precision);
-            $number *= $roundingCoef;
+            $number = (string) ($number * $roundingCoef);


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

jonny-no1 · 2017-03-02T11:30:46Z

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

fabpot · 2017-03-02T15:52:30Z

Thank you @foaly-nr1.

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); ``` From http://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 · 2017-03-02T15:52:58Z

Thanks @dmaicher for the nice trick!

carsonbot added Status: Needs Review Form Bug labels Feb 26, 2017

xabbuh added this to the 2.7 milestone Feb 26, 2017

dmaicher reviewed Mar 2, 2017

View reviewed changes

foaly-nr1 added 3 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 the issue-21734 branch from c3faf46 to 6ab0784 Compare March 2, 2017 11:20

fabpot closed this Mar 2, 2017

jonny-no1 deleted the issue-21734 branch March 2, 2017 15:54

This was referenced Mar 6, 2017

Release v2.7.25 #21884

Merged

Release v2.8.18 #21885

Merged

Release v3.2.5 #21939

Merged

nicolas-grekas mentioned this pull request Aug 31, 2017

[Form] Fix precision of MoneyToLocalizedStringTransformer's divisions and multiplications #24036

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

[Form] Improve rounding precision #21769

[Form] Improve rounding precision #21769

Uh oh!

jonny-no1 commented Feb 26, 2017 •

edited

Loading

Uh oh!

theofidry commented Feb 26, 2017

Uh oh!

javiereguiluz commented Feb 26, 2017

Uh oh!

jonny-no1 commented Feb 26, 2017

Uh oh!

nicolas-grekas commented Feb 28, 2017

Uh oh!

dmaicher commented Feb 28, 2017 •

edited

Loading

Uh oh!

dmaicher commented Feb 28, 2017 •

edited

Loading

Uh oh!

nicolas-grekas commented Mar 2, 2017 •

edited

Loading

Uh oh!

jonny-no1 commented Mar 2, 2017

Uh oh!

dmaicher Mar 2, 2017

Uh oh!

jonny-no1 commented Mar 2, 2017

Uh oh!

fabpot commented Mar 2, 2017

Uh oh!

fabpot commented Mar 2, 2017

Uh oh!

Uh oh!

Uh oh!

[Form] Improve rounding precision #21769

[Form] Improve rounding precision #21769

Uh oh!

Conversation

jonny-no1 commented Feb 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

theofidry commented Feb 26, 2017

Uh oh!

javiereguiluz commented Feb 26, 2017

Uh oh!

jonny-no1 commented Feb 26, 2017

Uh oh!

nicolas-grekas commented Feb 28, 2017

Uh oh!

dmaicher commented Feb 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dmaicher commented Feb 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nicolas-grekas commented Mar 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonny-no1 commented Mar 2, 2017

Uh oh!

dmaicher Mar 2, 2017

Choose a reason for hiding this comment

Uh oh!

jonny-no1 commented Mar 2, 2017

Uh oh!

fabpot commented Mar 2, 2017

Uh oh!

fabpot commented Mar 2, 2017

Uh oh!

Uh oh!

jonny-no1 commented Feb 26, 2017 •

edited

Loading

dmaicher commented Feb 28, 2017 •

edited

Loading

dmaicher commented Feb 28, 2017 •

edited

Loading

nicolas-grekas commented Mar 2, 2017 •

edited

Loading