Fix #12143: Improve precision of rounding for FP numbers by jorgsowa · Pull Request #12159 · php/php-src

jorgsowa · 2023-09-08T21:45:08Z

This PR fixes #12143

When we calculate FP numbers with large precision and perform floor(value + 0.5) or ceil(value + 0.5) we can round the result to the full integer which lead to incorrect result. We need to check if the number is not changed due to FP representation.

…PHP_ROUND_HALF_EVEN

TimWolla · 2023-09-09T15:54:47Z

ext/standard/tests/math/round_bug12143.phpt

+var_dump(round(0.49999999999999994, 0, PHP_ROUND_HALF_UP) == 0);
+var_dump(round(0.49999999999999994, 0, PHP_ROUND_HALF_DOWN) == 0);
+var_dump(round(0.49999999999999994, 0, PHP_ROUND_HALF_EVEN) == 0);
+var_dump(round(0.49999999999999994, 0, PHP_ROUND_HALF_ODD) == 0);
+var_dump(round(-0.49999999999999994, 0, PHP_ROUND_HALF_UP) == -0);
+var_dump(round(-0.49999999999999994, 0, PHP_ROUND_HALF_DOWN) == 0);
+var_dump(round(-0.49999999999999994, 0, PHP_ROUND_HALF_EVEN) == 0);
+var_dump(round(-0.49999999999999994, 0, PHP_ROUND_HALF_ODD) == 0);


It might be better to dump the resulting number, rather than the result of the comparison.

Changed it. And also added cases for the number 0.5000000000000004.

SakiTakamachi · 2023-09-09T16:24:17Z

@jorgsowa
Since this is a bug fix, the base branch should probably be PHP8.1 which is the oldest supported branch.
(This is my personal opinion, so please wait for maintainer's opinion before making your decision.)

TimWolla · 2023-09-10T17:46:18Z

For reference, here is a test program to easily check the behavior of php_round_helper with several numbers:

#include <math.h>
#include <float.h>
#include <stdio.h>

#ifndef PHP_ROUND_HALF_UP
#define PHP_ROUND_HALF_UP        0x01    /* Arithmetic rounding, up == away from zero */
#endif

#ifndef PHP_ROUND_HALF_DOWN
#define PHP_ROUND_HALF_DOWN      0x02    /* Down == towards zero */
#endif

#ifndef PHP_ROUND_HALF_EVEN
#define PHP_ROUND_HALF_EVEN      0x03    /* Banker's rounding */
#endif

#ifndef PHP_ROUND_HALF_ODD
#define PHP_ROUND_HALF_ODD       0x04
#endif

double php_round_helper(double value, int mode) {
	double tmp_value;

	if (value >= 0.0) {
		tmp_value = floor(value + 0.5);
		if ((mode == PHP_ROUND_HALF_DOWN && value == (-0.5 + tmp_value)) ||
			(mode == PHP_ROUND_HALF_EVEN && value == (0.5 + 2 * floor(tmp_value/2.0))) ||
			(mode == PHP_ROUND_HALF_ODD  && value == (0.5 + 2 * floor(tmp_value/2.0) - 1.0)))
		{
			tmp_value = tmp_value - 1.0;
		}
	} else {
		tmp_value = ceil(value - 0.5);
		if ((mode == PHP_ROUND_HALF_DOWN && value == (0.5 + tmp_value)) ||
			(mode == PHP_ROUND_HALF_EVEN && value == (-0.5 + 2 * ceil(tmp_value/2.0))) ||
			(mode == PHP_ROUND_HALF_ODD  && value == (-0.5 + 2 * ceil(tmp_value/2.0) + 1.0)))
		{
			tmp_value = tmp_value + 1.0;
		}
	}

	return tmp_value;
}

int
main(void) {
	double d;
	double values[] = { 0.5, 1.5, 2.5, 3.5, 32.5, 64.5, 1024.5, 2048.5, 4096.5, 4503599627370494.5, 4503599627370495.5, 4503599627370496.5, 9007199254740989.5, 9007199254740990.5, 9007199254740991.5, 9007199254740992.5 };

	for (size_t i = 0; i < sizeof(values) / sizeof(values[0]); i++) {
		double value = values[i];
		d = nextafter(value, 0); printf("%.17f %.17f %.17f\n", d, php_round_helper(d, PHP_ROUND_HALF_UP), php_round_helper(d, PHP_ROUND_HALF_DOWN));
		d = value; printf("%.17f %.17f %.17f\n", d, php_round_helper(d, PHP_ROUND_HALF_UP), php_round_helper(d, PHP_ROUND_HALF_DOWN));
		d = nextafter(value, DBL_MAX); printf("%.17f %.17f %.17f\n", d, php_round_helper(d, PHP_ROUND_HALF_UP), php_round_helper(d, PHP_ROUND_HALF_DOWN));
		printf("\n");
	}
}

SakiTakamachi · 2023-09-12T00:59:32Z

ext/standard/tests/math/round_bug12143.phpt

+var_dump(round(0.5000000000000004, 0, PHP_ROUND_HALF_UP));
+var_dump(round(0.5000000000000004, 0, PHP_ROUND_HALF_DOWN));
+var_dump(round(0.5000000000000004, 0, PHP_ROUND_HALF_EVEN));
+var_dump(round(0.5000000000000004, 0, PHP_ROUND_HALF_ODD));
+var_dump(round(-0.5000000000000004, 0, PHP_ROUND_HALF_UP));
+var_dump(round(-0.5000000000000004, 0, PHP_ROUND_HALF_DOWN));
+var_dump(round(-0.5000000000000004, 0, PHP_ROUND_HALF_EVEN));
+var_dump(round(-0.5000000000000004, 0, PHP_ROUND_HALF_ODD));


[IMO]

The internal representation of double precision around 0.5 in IEEE754 and the corresponding FP are as follows:

3fdfffffffffffff // 0.49999999999999994 3fe0000000000000 // 0.5 3fe0000000000001 // 0.50000000000000001

Therefore, considering the purpose of the test, it would be better to set the value as follows(There were some places where digits were missing):

Suggested change

var_dump(round(0.5000000000000004, 0, PHP_ROUND_HALF_UP));

var_dump(round(0.5000000000000004, 0, PHP_ROUND_HALF_DOWN));

var_dump(round(0.5000000000000004, 0, PHP_ROUND_HALF_EVEN));

var_dump(round(0.5000000000000004, 0, PHP_ROUND_HALF_ODD));

var_dump(round(-0.5000000000000004, 0, PHP_ROUND_HALF_UP));

var_dump(round(-0.5000000000000004, 0, PHP_ROUND_HALF_DOWN));

var_dump(round(-0.5000000000000004, 0, PHP_ROUND_HALF_EVEN));

var_dump(round(-0.5000000000000004, 0, PHP_ROUND_HALF_ODD));

var_dump(round(0.5, 0, PHP_ROUND_HALF_UP));

var_dump(round(0.5, 0, PHP_ROUND_HALF_DOWN));

var_dump(round(0.5, 0, PHP_ROUND_HALF_EVEN));

var_dump(round(0.5, 0, PHP_ROUND_HALF_ODD));

var_dump(round(-0.5, 0, PHP_ROUND_HALF_UP));

var_dump(round(-0.5, 0, PHP_ROUND_HALF_DOWN));

var_dump(round(-0.5, 0, PHP_ROUND_HALF_EVEN));

var_dump(round(-0.5, 0, PHP_ROUND_HALF_ODD));

var_dump(round(0.50000000000000001, 0, PHP_ROUND_HALF_UP));

var_dump(round(0.50000000000000001, 0, PHP_ROUND_HALF_DOWN));

var_dump(round(0.50000000000000001, 0, PHP_ROUND_HALF_EVEN));

var_dump(round(0.50000000000000001, 0, PHP_ROUND_HALF_ODD));

var_dump(round(-0.50000000000000001, 0, PHP_ROUND_HALF_UP));

var_dump(round(-0.50000000000000001, 0, PHP_ROUND_HALF_DOWN));

var_dump(round(-0.50000000000000001, 0, PHP_ROUND_HALF_EVEN));

var_dump(round(-0.50000000000000001, 0, PHP_ROUND_HALF_ODD));

We may also want to check 1.5 and 2.5 as well to confirm the operation of ODD and EVEN.

[FYI]

These internal representations and their corresponding FP are:

3ff7ffffffffffff // 1.4999999999999998 3ff8000000000000 // 1.5 3ff8000000000001 // 1.5000000000000002

4003ffffffffffff // 2.4999999999999996 4004000000000000 // 2.5 4004000000000001 // 2.5000000000000004

bukka · 2023-09-15T09:22:09Z

Doesn't this have the same effect as #12162 ? As I understand it the last part of condition is pretty much fallback to simple comparison check. Or will the previous mode specific checks have any sensible impact when used in pre-rounding?

I assume that the additional check is only useful in pre-rounding, right? If so, it might be a slight waste to do it all the time.

SakiTakamachi · 2023-09-15T09:38:38Z

@bukka
This fixes improper rounding due to floating point precision issues.

For example, it is wrong to round 0.49999999999999994 to 1.
However, the existing processing looks like this:

0.49999999999999994 + 0.5
= 0.99999999999999994 // this is 3ff0000000000000 in IEEE754 with double precision.
= 1

Double precision makes it impossible to distinguish between 0.99999999999999994 and 1, so it is incorrectly rounded to 1.

Also, 0.49999999999999994 is a number that is not affected by #12162, which is a completely different problem.

bukka · 2023-09-15T11:31:10Z

Ah ok I see now. It makes sense

This change makes the implementation much easier to understand, by explicitly handling the various cases. It fixes rounding for `0.49999999999999994`, because no loss of precision happens by adding / subtracing `0.5` before turning the result into an integral float. Instead the fractional parts are explicitly compared. see phpGH-12143 (this fixes one of the reported cases) Closes phpGH-12159 which was an alternative attempt to fix the rounding issue for `0.49999999999999994`

This change makes the implementation much easier to understand, by explicitly handling the various cases. It fixes rounding for `0.49999999999999994`, because no loss of precision happens by adding / subtracing `0.5` before turning the result into an integral float. Instead the fractional parts are explicitly compared. see GH-12143 (this fixes one of the reported cases) Closes GH-12159 which was an alternative attempt to fix the rounding issue for `0.49999999999999994`

Change precision of rounding for large FP numbers:

6c474eb

github-actions bot added the Extension: standard label Sep 8, 2023

jorgsowa added 3 commits September 9, 2023 00:16

Fix rounding for large precision FP numbers for PHP_ROUND

efd8e7d

Improve the rounding for FP numbers for modes PHP_ROUND_HALF_ODD and …

112d9d4

…PHP_ROUND_HALF_EVEN

Removed changes to prerounding

f762b8b

jorgsowa changed the title ~~Fix #12123 Improve precision of rounding for large FP numbers~~ Fix #12123: Improve precision of rounding for FP numbers Sep 9, 2023

jorgsowa marked this pull request as ready for review September 9, 2023 14:19

jorgsowa requested a review from bukka as a code owner September 9, 2023 14:19

jorgsowa mentioned this pull request Sep 9, 2023

Incorrect round($num, 0, PHP_ROUND_HALF_UP) result for $num = 1.4999999999999998 / 4503599627370495.5 #12143

Closed

TimWolla reviewed Sep 9, 2023

View reviewed changes

Added more numbers to test

f043c10

jorgsowa changed the title ~~Fix #12123: Improve precision of rounding for FP numbers~~ Fix #12143: Improve precision of rounding for FP numbers Sep 10, 2023

SakiTakamachi reviewed Sep 12, 2023

View reviewed changes

TimWolla mentioned this pull request Sep 15, 2023

Reimplement php_round_helper() using modf() #12220

Merged

TimWolla closed this in #12220 Sep 19, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #12143: Improve precision of rounding for FP numbers#12159

Fix #12143: Improve precision of rounding for FP numbers#12159
jorgsowa wants to merge 5 commits intophp:masterfrom
jorgsowa:fix-gh-12143

jorgsowa commented Sep 8, 2023 •

edited

Loading

Uh oh!

TimWolla Sep 9, 2023

Uh oh!

jorgsowa Sep 10, 2023

Uh oh!

SakiTakamachi commented Sep 9, 2023 •

edited

Loading

Uh oh!

TimWolla commented Sep 10, 2023 •

edited

Loading

Uh oh!

SakiTakamachi Sep 12, 2023

Uh oh!

SakiTakamachi Sep 12, 2023

Uh oh!

bukka commented Sep 15, 2023 •

edited

Loading

Uh oh!

SakiTakamachi commented Sep 15, 2023 •

edited

Loading

Uh oh!

bukka commented Sep 15, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

-var_dump(round(0.5000000000000004, 0, PHP_ROUND_HALF_UP));
-var_dump(round(0.5000000000000004, 0, PHP_ROUND_HALF_DOWN));
-var_dump(round(0.5000000000000004, 0, PHP_ROUND_HALF_EVEN));
-var_dump(round(0.5000000000000004, 0, PHP_ROUND_HALF_ODD));
-var_dump(round(-0.5000000000000004, 0, PHP_ROUND_HALF_UP));
-var_dump(round(-0.5000000000000004, 0, PHP_ROUND_HALF_DOWN));
-var_dump(round(-0.5000000000000004, 0, PHP_ROUND_HALF_EVEN));
-var_dump(round(-0.5000000000000004, 0, PHP_ROUND_HALF_ODD));
+var_dump(round(0.5, 0, PHP_ROUND_HALF_UP));
+var_dump(round(0.5, 0, PHP_ROUND_HALF_DOWN));
+var_dump(round(0.5, 0, PHP_ROUND_HALF_EVEN));
+var_dump(round(0.5, 0, PHP_ROUND_HALF_ODD));
+var_dump(round(-0.5, 0, PHP_ROUND_HALF_UP));
+var_dump(round(-0.5, 0, PHP_ROUND_HALF_DOWN));
+var_dump(round(-0.5, 0, PHP_ROUND_HALF_EVEN));
+var_dump(round(-0.5, 0, PHP_ROUND_HALF_ODD));
+var_dump(round(0.50000000000000001, 0, PHP_ROUND_HALF_UP));
+var_dump(round(0.50000000000000001, 0, PHP_ROUND_HALF_DOWN));
+var_dump(round(0.50000000000000001, 0, PHP_ROUND_HALF_EVEN));
+var_dump(round(0.50000000000000001, 0, PHP_ROUND_HALF_ODD));
+var_dump(round(-0.50000000000000001, 0, PHP_ROUND_HALF_UP));
+var_dump(round(-0.50000000000000001, 0, PHP_ROUND_HALF_DOWN));
+var_dump(round(-0.50000000000000001, 0, PHP_ROUND_HALF_EVEN));
+var_dump(round(-0.50000000000000001, 0, PHP_ROUND_HALF_ODD));

Conversation

jorgsowa commented Sep 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TimWolla Sep 9, 2023

Choose a reason for hiding this comment

Uh oh!

jorgsowa Sep 10, 2023

Choose a reason for hiding this comment

Uh oh!

SakiTakamachi commented Sep 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TimWolla commented Sep 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SakiTakamachi Sep 12, 2023

Choose a reason for hiding this comment

Uh oh!

SakiTakamachi Sep 12, 2023

Choose a reason for hiding this comment

Uh oh!

bukka commented Sep 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SakiTakamachi commented Sep 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bukka commented Sep 15, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jorgsowa commented Sep 8, 2023 •

edited

Loading

SakiTakamachi commented Sep 9, 2023 •

edited

Loading

TimWolla commented Sep 10, 2023 •

edited

Loading

bukka commented Sep 15, 2023 •

edited

Loading

SakiTakamachi commented Sep 15, 2023 •

edited

Loading