Closed pixelzoom closed 9 years ago
The desired behavior (which I've implemented in Util.toFixed) is IEEE 754 "Round half away from zero" algorithm, see https://en.wikipedia.org/wiki/Rounding#Round_half_away_from_zero. It treats positive and negative values symmetrically.
It occurred to me that it would be nice to have a round function that behaves this way.
Work completed. I added Util.roundSymmetric, and used it in Util.toFixed. I added documentation about the problem with Math.round, and references to the relevant IEEE 754 specs.
Assigning to @jonathanolson to review.
Summary for developers, which I also sent in an email and Skype:
JavaScript’s Math.round uses “round half up” (https://en.wikipedia.org/wiki/Rounding#Round_half_up), which does not treat positive and negative numbers symmetrically.
Example:
> Math.round( 1.5 )
2
> Math.round( -1.5 )
-1
That behavior is problematic for PhET sims, where we want to treat positive and negative numbers symmetrically.
I’ve added dot.Util.roundSymmetric, which treats positive and negative numbers symmetrically and uses "round half away from zero“ (https://en.wikipedia.org/wiki/Rounding#Round_half_away_from_zero).
Example:
> Util.roundSymmetric( 1.5 )
2
> Util.roundSymmetric( -1.5 )
-2
Util.roundSymmetric is also used in Util.toFixed, so that formatting of numbers will also be treat positive and negative numbers symmetrically.
Example:
> Util.toFixed( 1.56, 1 )
“1.6”
> Util.toFixed( -1.56, 1 )
“-1.6"
Recommendations:
• Don’t use Math.round unless you want “round half up” behavior • Inspect uses of Math.round in existing sim code, and change to Util.roundSymmetric where appropriate • Look for inappropriate uses of Math.round during code reviews
Re:
• Look for inappropriate uses of Math.round during code reviews
I've added this section to the Code Review Checklist (https://github.com/phetsims/phet-info/blob/master/code_review_checklist.md):
Common Errors
Math.random
used where dot.Util.randomSymmetric
should be used? Math.random does not treat positive and negative numbers symmetrically, see https://github.com/phetsims/dot/issues/35#issuecomment-113587879toFixed
used where dot.Util.toFixed
or dot.Util.toFixedNumber
should be used? JavaScript's toFixed
is notoriously buggy, behavior differs depending on browser, because the spec doesn't specify whether to round or floor.Re:
• Inspect uses of Math.round in existing sim code, and change to Util.roundSymmetric where appropriate
Addressing in https://github.com/phetsims/tasks/issues/275.
Looks good to me.
The only case where it really otherwise deviates seems to be -0:
Math.round( -0 ); // -0
roundSymmetric( -0 ); // 0
Feel free to close if it was just pending on review.
@jonathanolson I purposely did not round -0 to -0 because generally we don't want to display -0 in sims. Do you disagree? Is the issue only with -0, or do you think that -1.2 should be rounded to -0?
I purposely did not round -0 to -0 because generally we don't want to display -0 in sims. Do you disagree? Is the issue only with -0, or do you think that -1.2 should be rounded to -0?
Sounds fine to me, just wanted to note it.
I've noted the -0 behavior in the documentation, https://github.com/phetsims/dot/commit/65da3f573d6d13ae776d93aed25eed022079f33d
Closing.
Behavior of Math.round (and therefore dot.Util.toFixed) is currently:
Math.round(1.5) -> 2, Math.round(-1.5) -> -1.
This is not desirable for PhET sims. The desired behavior is one that treats positive and negative values symmetrically. Take care of this in dot.Util.toFixed.