r46671 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r46670‎ | r46671 | r46672 >
Date:03:56, 1 February 2009
Author:rarohde
Status:reverted (Comments)
Tags:
Comment:
Adds explicit round-off checking to operations that are sensitive to floating point vs. integer round-off errors.

Where applied, it assumes that any floating point number that is an integer to within one part in 10^10 should be replaced with that integer.
Modified paths:
  • /trunk/extensions/ParserFunctions/Expr.php (modified) (history)

Diff [purge]

Index: trunk/extensions/ParserFunctions/Expr.php
——@@ -153,6 +153,29 @@
154154 );
155155
156156 /**
 157+ * Checks if $expr is an integer within one part in 10^10.
 158+ * If so, recast as integer and return, else return $expr unchanged.
 159+ *
 160+ * Explicit checking for round-off errors should eliminate
 161+ * many bugs associated with floating point conversion to integers.
 162+ */
 163+ function checkInteger( $expr ) {
 164+ $intval = (int)$expr;
 165+
 166+ if( $expr >= 0 ) {
 167+ if( $expr - $intval > 0.9999999999 ) { $intval += 1; }
 168+ } else {
 169+ if( $expr - $intval < -0.9999999999 ) { $intval -= 1; }
 170+ }
 171+
 172+ if( abs( $intval - $expr ) < 0.0000000001 ) {
 173+ return $intval;
 174+ } else {
 175+ return $expr;
 176+ }
 177+ }
 178+
 179+ /**
157180 * Evaluate a mathematical expression
158181 *
159182 * The algorithm here is based on the infix to RPN algorithm given in
——@@ -400,7 +423,7 @@
401424 $right = array_pop( $stack );
402425 $left = array_pop( $stack );
403426 if ( $right == 0 ) throw new ExprError('division_by_zero', $this->names[$op]);
404 - $stack[] = $left % $right;
 427+ $stack[] = $this->checkInteger( $left ) % $this->checkInteger( $right );
405428 break;
406429 case EXPR_PLUS:
407430 if ( count( $stack ) < 2 ) throw new ExprError('missing_operand', $this->names[$op]);
——@@ -430,7 +453,7 @@
431454 if ( count( $stack ) < 2 ) throw new ExprError('missing_operand', $this->names[$op]);
432455 $right = array_pop( $stack );
433456 $left = array_pop( $stack );
434 - $stack[] = ( $left == $right ) ? 1 : 0;
 457+ $stack[] = ( $this->checkInteger( $left ) == $this->checkInteger( $right ) ) ? 1 : 0;
435458 break;
436459 case EXPR_NOT:
437460 if ( count( $stack ) < 1 ) throw new ExprError('missing_operand', $this->names[$op]);
——@@ -441,43 +464,43 @@
442465 if ( count( $stack ) < 2 ) throw new ExprError('missing_operand', $this->names[$op]);
443466 $digits = intval( array_pop( $stack ) );
444467 $value = array_pop( $stack );
445 - $stack[] = round( $value, $digits );
 468+ $stack[] = round( $this->checkInteger( $value ), $digits );
446469 break;
447470 case EXPR_LESS:
448471 if ( count( $stack ) < 2 ) throw new ExprError('missing_operand', $this->names[$op]);
449472 $right = array_pop( $stack );
450473 $left = array_pop( $stack );
451 - $stack[] = ( $left < $right ) ? 1 : 0;
 474+ $stack[] = ( $this->checkInteger( $left ) < $this->checkInteger( $right ) ) ? 1 : 0;
452475 break;
453476 case EXPR_GREATER:
454477 if ( count( $stack ) < 2 ) throw new ExprError('missing_operand', $this->names[$op]);
455478 $right = array_pop( $stack );
456479 $left = array_pop( $stack );
457 - $stack[] = ( $left > $right ) ? 1 : 0;
 480+ $stack[] = ( $this->checkInteger( $left ) > $this->checkInteger( $right ) ) ? 1 : 0;
458481 break;
459482 case EXPR_LESSEQ:
460483 if ( count( $stack ) < 2 ) throw new ExprError('missing_operand', $this->names[$op]);
461484 $right = array_pop( $stack );
462485 $left = array_pop( $stack );
463 - $stack[] = ( $left <= $right ) ? 1 : 0;
 486+ $stack[] = ( $this->checkInteger( $left ) <= $this->checkInteger( $right ) ) ? 1 : 0;
464487 break;
465488 case EXPR_GREATEREQ:
466489 if ( count( $stack ) < 2 ) throw new ExprError('missing_operand', $this->names[$op]);
467490 $right = array_pop( $stack );
468491 $left = array_pop( $stack );
469 - $stack[] = ( $left >= $right ) ? 1 : 0;
 492+ $stack[] = ( $this->checkInteger( $left ) >= $this->checkInteger( $right ) ) ? 1 : 0;
470493 break;
471494 case EXPR_NOTEQ:
472495 if ( count( $stack ) < 2 ) throw new ExprError('missing_operand', $this->names[$op]);
473496 $right = array_pop( $stack );
474497 $left = array_pop( $stack );
475 - $stack[] = ( $left != $right ) ? 1 : 0;
 498+ $stack[] = ( $this->checkInteger( $left ) != $this->checkInteger( $right ) ) ? 1 : 0;
476499 break;
477500 case EXPR_EXPONENT:
478501 if ( count( $stack ) < 2 ) throw new ExprError('missing_operand', $this->names[$op]);
479502 $right = array_pop( $stack );
480503 $left = array_pop( $stack );
481 - $stack[] = $left * pow(10,$right);
 504+ $stack[] = $left * pow(10, $this->checkInteger( $right ) );
482505 break;
483506 case EXPR_SINE:
484507 if ( count( $stack ) < 1 ) throw new ExprError('missing_operand', $this->names[$op]);
——@@ -530,17 +553,17 @@
531554 case EXPR_FLOOR:
532555 if ( count( $stack ) < 1 ) throw new ExprError('missing_operand', $this->names[$op]);
533556 $arg = array_pop( $stack );
534 - $stack[] = floor($arg);
 557+ $stack[] = floor( $this->checkInteger( $arg ) );
535558 break;
536559 case EXPR_TRUNC:
537560 if ( count( $stack ) < 1 ) throw new ExprError('missing_operand', $this->names[$op]);
538561 $arg = array_pop( $stack );
539 - $stack[] = (int)$arg;
 562+ $stack[] = (int)( $this->checkInteger( $arg ) );
540563 break;
541564 case EXPR_CEIL:
542565 if ( count( $stack ) < 1 ) throw new ExprError('missing_operand', $this->names[$op]);
543566 $arg = array_pop( $stack );
544 - $stack[] = ceil($arg);
 567+ $stack[] = ceil( $this->checkInteger( $arg ) );
545568 break;
546569 case EXPR_POW:
547570 if ( count( $stack ) < 2 ) throw new ExprError('missing_operand', $this->names[$op]);

Follow-up revisions

RevisionCommit summaryAuthorDate
r46683Adds explicit fractional tolerance checks to comparison functions. Currently...rarohde18:40, 1 February 2009
r47200Revert "Adds explicit round-off checking to operations that are sensitive to ...werdna22:29, 12 February 2009

Comments

#Comment by Simetrical (talk | contribs)   04:00, 1 February 2009

Doesn't this mean that it's impossible to do any operations to a precision of more than 10^-10? If users want this, they should do the rounding themselves using "round". I don't think reducing the accuracy of the functions is the right way to fix the problem: if anything, we want to *increase* it, preferably to levels suitable for all physics-related applications (which would probably require exact decimals).

#Comment by Dragons flight (talk | contribs)   04:09, 1 February 2009

The rounding is only applied to functions like mod, ceil, and comparison functions, where numbers like 5.99999999999 should nearly always be understood as 6.

Normal operations like *, +, - are not affected.

#Comment by Simetrical (talk | contribs)   15:55, 1 February 2009

(copied from wikitech-l)

However, this causes problems for expressions like

{{#expr:1e-11 = 2e-11}}

This is now (wrongly) true, while before your commit it was false. In the old way, at least users could work around the problem in the same way that programmers do: by explicit testing like

{{#expr:0.00007 * 10000000 - 700 < 1e-10}}

Now I don't think there's any way to get correct results in cases where small numbers are entered manually. Plus, it doesn't help stuff like

{{#expr:0.00000007 * 10000000 = 0.7}}

which still gives wrong results.

#Comment by Dragons flight (talk | contribs)   04:29, 1 February 2009

To give some examples of the issue, right now:

0.07*100 == 7 is False (0.00007*1000*1000) % 1000 => 69

etc.

#Comment by Dragons flight (talk | contribs)   18:44, 1 February 2009

Modified in r46683

#Comment by Werdna (talk | contribs)   19:17, 17 February 2009

Reverted in r47200 and friends, in favour of a better solution.

Status & tagging log