inspect uses of Math.round

[pixelzoom]

As discovered in https://github.com/phetsims/dot/issues/35, Math.round is not symmetric for positive and negative numbers, and that's not desirable for PhET sims. Typically dot.Util.roundSymmetric should be used instead.

There are currently 178 occurrences of Math.round in PhET code. These should be inspected and changed to dot.Util.roundSymmetric where appropriate. Where Math.round is OK, recommended to add a comment saying why "round half up" is preferred (unless it's obvious that numbers are positive only).

Recommend that each developer handle this for sims that they were responsible for. I'll start, then reassign to someone else.

[pixelzoom]

I inspected Math.round in the following repos:

acid-base-solutions (all OK, positive numbers only) beers-law-lab (all OK, positive numbers only) dot (used only in Util.roundSymmetric) graphing-lines (9 occurrences all changed to Util.roundSymmetric) hookes-law (4 occurrences all changed to Util.roundSymmetric) joist (all OK, positive numbers only) molarity (all OK, positive numbers only) ph-scale (all OK, positive numbers only) phetcommon (2 occurrences in Fraction.js change to Util.roundSymmetric) reactants-products-and-leftovers (all OK, positive numbers only) scenery-phet (all OK, positive numbers only)

[pixelzoom]

Assigning to @jbphet to inspect his sims.

[samreid]

Would there be a problem with just replacing all occurrences? Or are there some cases where Math.round is necessary?

[pixelzoom]

If Math.round is used in a performance-critical loop, changing to Util.roundSymmetric will incur 2 additional function calls - the call to Util.roundSymmetric and a call to Math.abs.

[jbphet]

I've reviewed and updated the following sims:

• area-builder
• arithmetic
• atomic-interactions
• balancing-act
• beaker
• build-an-atom
• energy-forms-and-changes
• estimation
• faradays-law
• friction
• gene-expression-basics
• gravity-force-lab
• isotopes-and-atomic-mass
• molecules-and-light
• neuron
• ohms-law
• resistance-in-a-wire
• states-of-matter
• states-of-matter-basics
• under-pressure
• vegas
• vibe

Assigning to @jonathanolson for the next round.

[samreid]

If Math.round is used in a performance-critical loop, changing to Util.roundSymmetric will incur 2 additional function calls - the call to Util.roundSymmetric and a call to Math.abs.

I agree but wanted to document some thoughts I had about this.

The implementation is:

    roundSymmetric: function( value ) {
      return ( ( value < 0 ) ? -1 : 1 ) * Math.round( Math.abs( value ) );
    }

So this looks very fast and optimizable (by the browser).

Also, rounding seems unlikely to be used in a performance critical loop--for instance, performance critical loops for physics updates would be unlikely to use rounding as part of the math (full precision is likely to be used instead) and rounding is unlikely to be applied during the loop. Rounding typically happens less frequently and right before formatting a value for display in the UI.

[jonathanolson]

Took care of sims I've done or liaison'ed, assigning to @samreid for his sims.

[samreid]

I reviewed usages of Math.round in:

balloons-and-static-electricity beaker bending-light energy-skate-park-basics forces-and-motion-basics fluid-pressure-and-flow

the latter two will need attention.

[samreid]

I determined that all at-risk usages in fluid-pressure-and-flow are positive, so that's fine.

[samreid]

I finished the work for the sims and libraries for which I am the primary developer. Next, over to @aaronsamuel137

[aaronsamuel137]

I review usages in color-vision and gravity-and-orbits. Assigning to @jessegreenberg.

[jessegreenberg]

It looks like @jbphet already inspected molecules-and-light, energy-forms-and-changes, and isotopes-and-atomic-mass. I looked over the updates made to those sims.

I reviewed usages thus far in capacitor-lab-basics, all rounding operations on clearly positive values.

I believe that is everyone, reassigning to @pixelzoom to review, delegate next steps, or close.

[pixelzoom]

Reopening. I believe we forgot @bereaphysics. Martin, if you don't have time for this, assign back to me and I'll review your sims.

[pixelzoom]

I also see that @dubson has a number of questionable uses of Math.round in trig-lab.

[veillette]

Will do

[pixelzoom]

In https://github.com/phetsims/tasks/issues/275#issuecomment-114193425 @samreid asked:

Would there be a problem with just replacing all occurrences? Or are there some cases where Math.round is necessary?

I'm warming up to this suggestion. Currently we still have 145 uses of Math.round, and more will undoubtedly be created in the future by developers who are not familiar with this issues (see trig-lab for recent concrete examples.) It would be much easier to say "don't use Math.round unless you have a specific reason to use it, and have documented why you want non-symmetric rounding".

[pixelzoom]

A similar argument applies to toFixed.

[veillette]

Personally, I use math.ceil or math.floor to convert float to integer. This is often used in some intermediate calculation step and is not necessarily used to format a number to be displayed on the screen. If someone would use round instead, then symmetric rounding would probably not lead to the intended result. Personally, I would advocate the use of roundSymmetric on a need of use basis.

[veillette]

I reviewed the following sims

• charges-and-fields
• blackbody-spectrum
• least-squares-regression
• plinko-probability

Blackbody spectrum was the only sim making use of Math.round. It was changed to Util.roundSymmetric

Reassigning to @pixelzoom for further assignment.

[pixelzoom]

I think that's everyone.

Since there is no consensus on whether to use Util.roundSymmetric everywhere, we won't do that. The "Common Errors" section of the code review checklist (https://github.com/phetsims/phet-info/blob/master/code_review_checklist.md) includes reviewing uses of Math.round and toFixed, so hopefully that will prevent inappropriate uses of those functions from creeping back in.

Closing.