Utils.toFixed returns incorrect results in some cases

[pixelzoom]

Discovered while working on https://github.com/phetsims/scenery-phet/issues/747.

Utils.toFixed was created as a workaround for problems with built-in toFixed, which rounds differently on different browsers.

Here's the current implementation:

  /**
   * A predictable implementation of toFixed.
   * @public
   *
   * JavaScript's toFixed is notoriously buggy, behavior differs depending on browser,
   * because the spec doesn't specify whether to round or floor.
   * Rounding is symmetric for positive and negative values, see Utils.roundSymmetric.
   *
   * @param {number} value
   * @param {number} decimalPlaces
   * @returns {string}
   */
  toFixed( value, decimalPlaces ) {
    const multiplier = Math.pow( 10, decimalPlaces );
    const newValue = Utils.roundSymmetric( value * multiplier ) / multiplier;
    return newValue.toFixed( decimalPlaces ); // eslint-disable-line bad-sim-text
  },

But Utils.toFixed does not return the correct value in all cases. It's subject to floating-point error.

For example, this should be '35.86':

> phet.dot.Utils.toFixed( 35.855, 2 )
'35.85'

This is caused by value * multiplier in the implementation of Utils.toFixed:

> 35.855 * 100
3585.4999999999995

This problem could result in widespread errors in PhET sims. It's certainly affecting ScientificNotationNode.

Do we want to do anything about this? Can we do anything about this?

[pixelzoom]

In the above commits, I added toFixedPointString.ts, with unit tests in toFixedPointStringTests.js. (I decided to put this function in its own file, since Util.js has not been converted to TypeScript.)

Function toFixedPointString has the same API as Number.toFixed and Util.toFixed, but performs no math operations. It does all of its work by manipulating strings. So it does not have the rounding and floating-point problems inherent in Number.toFixed and Utils.toFixed.

I'm planning to use this in ScientificNotationNode. And I'd like to discuss whether this should be a replacement for Utils.toFixed.

[zepumph]

From dev meeting today, we will wait to discuss with @pixelzoom and @jonathanolson.

[jessegreenberg]

@liam-mulhall suggested a library like math.js that may have support for handling this. @pixelzoom: Yes, we have discussed before. It could be a big change, we would have to use that any time we work with numbers. But it could be good to investigate. @zepumph: Can we use a library in specific cases instead of using broadly? @kathy-phet @pixelzoom @zepumph: Maybe we could use Math.js just in Utils.toFixed at first.

@pixelzoom: Since this is a systemic problem, should anything be done for ScientificNotationNode and build-a-nucleus? @kathy-phet: That is not necessary, if we are going to address this it should be done generally.

@pixelzoom wrote a string manipulation algorithm to get around this in https://github.com/phetsims/dot/issues/113#issuecomment-1163780635. But he found it couldn't be used for ScientificNotationNode because division was required before the string manipulation. @zepumph: Can we change Utils.toFixed to use this? @pixelzoom: Worried that it is not as performant as current implementation of Utils.toFixed.

@kathy-phet: Lets bring this up during quarterly goals discussion, this might be a good project to work on generally.

[zepumph]

This investigation should be handled over in the epic issue in https://github.com/phetsims/dot/issues/116

[samreid]

This StackOverflow post refers to an MDN polyfill implementation they describe as "reliable rounding implementation". https://stackoverflow.com/questions/42109818/reliable-js-rounding-numbers-with-tofixed2-of-a-3-decimal-number.

I tested it in the browser console and it appeared to have the correct behavior for the error case above (same good behavior on Chrome and Firefox):

Math.round10(35.855, -2);
35.86

Note this returns a number instead of a string, so would still need one more step before it could be used for toFixed. I'm not sure if it addresses the case for ScientificNotationNode or not.

Also, we would want to adapt it to run as a regular function instead of monkey patching Math.

[pixelzoom]

At 7/6/2022 quarterly-planning meeting, this was assigned to me for next steps, starting with the polyfill that @samreid identified in https://github.com/phetsims/dot/issues/113#issuecomment-1175314381.

[marlitas]

This is on @pixelzoom's list. It is unclear that it needs to be in subgroups. We are moving to done discussing. Feel free to move it back to subgroups for any reason if you'd like.

[pixelzoom]

I haven't gotten to this in > 1 year. It's not a priority, and (honestly) not really something that I'm interested in. So unassigning and reverting to the "To Be Discussed" column on the Developer Meeting project board.

[marlitas]

@pixelzoom I think the Developer Meeting project board is no longer active. I'll add this as a discussion topic for the next developer meeting and we can discuss priority of allocating resources to address it in the future.

[zepumph]

@samreid mentioned it may be pretty straight forward when calling toFixed to assert that it is the same as toFixedPointString and then run local fuzzing to see if there are any instances in which they differ, to understand the scope of the bugginess of Utils.toFixed().

We aren't really sure on the priority though.

[samreid]

I'm testing this patch in aqua:

```diff Subject: [PATCH] Update documentation, see https://github.com/phetsims/projectile-data-lab/issues/7 --- Index: js/Utils.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/Utils.js b/js/Utils.js --- a/js/Utils.js (revision da8f04fb4ba29c460000ffedb9716350896dafa1) +++ b/js/Utils.js (date 1702580013533) @@ -551,9 +551,50 @@ * @returns {string} */ toFixed( value, decimalPlaces ) { + const multiplier = Math.pow( 10, decimalPlaces ); const newValue = Utils.roundSymmetric( value * multiplier ) / multiplier; - return newValue.toFixed( decimalPlaces ); // eslint-disable-line bad-sim-text + const regularToFixed = newValue.toFixed( decimalPlaces ); // eslint-disable-line bad-sim-text + + const newToFixed = Utils.toFixedCustom( value, decimalPlaces ); + console.log( regularToFixed === newToFixed, regularToFixed, newToFixed ); + + return regularToFixed; + }, + + /** + * Decimal adjustment of a number. + * + * @param {String} type The type of adjustment. + * @param {Number} value The number. + * @param {Integer} exp The exponent (the 10 logarithm of the adjustment base). + * @returns {Number} The adjusted value. + */ + decimalAdjust( type, value, exp ) { + // If the exp is undefined or zero... + if ( typeof exp === 'undefined' || +exp === 0 ) { + return Math[ type ]( value ); + } + value = +value; + exp = +exp; + // If the value is not a number or the exp is not an integer... + if ( isNaN( value ) || !( typeof exp === 'number' && exp % 1 === 0 ) ) { + return NaN; + } + // Shift + value = value.toString().split( 'e' ); + value = Math[ type ]( +( value[ 0 ] + 'e' + ( value[ 1 ] ? ( +value[ 1 ] - exp ) : -exp ) ) ); + // Shift back + value = value.toString().split( 'e' ); + return +( value[ 0 ] + 'e' + ( value[ 1 ] ? ( +value[ 1 ] + exp ) : exp ) ); + }, + + round10( value, exp ) { + return Utils.decimalAdjust( 'round', value, exp ); + }, + + toFixedCustom( value, digits ) { + return Utils.round10( value, -digits ).toFixed( digits ); }, /** ```

I'm through the letter 'c' and so far all values are the same. I tested the value pointed out above and saw that it corrected the problem:

phet.dot.Utils.toFixed( 35.855, 2 )
> false '35.85' '35.86'

[zepumph]

@samreid and I will discuss the discrepancies that he is finding.

[samreid]

Results of local aqua:

``` Sim errors (dev): collision-lab Uncaught Error: Assertion failed: toFixedCustom should match regular toFixed, 0.00 !== -0.01 collision-lab Uncaught Error: Assertion failed: reentry detected, value=Vector2(-0.0050000000000003375, 0.495), oldValue=Vector2(-0.005, 0.495) Error: Assertion failed: toFixedCustom should match regular toFixed, 0.00 !== -0.01 at window.assertions.assertFunction (http://localhost/assert/js/assert.js:28:13) at assert (Utils.js:561:14) at toFixed (BallValuesPanelNumberDisplay.js:83:31) at numberFormatter (NumberDisplay.ts:395:18) at valueToString (NumberDisplay.ts:219:28) at derivation (DerivedProperty.ts:51:11) at getDerivedValue (DerivedProperty.ts:174:17) at listener (TinyEmitter.ts:123:8) at emit (ReadOnlyProperty.ts:329:22) at _notifyListeners (ReadOnlyProperty.ts:276:13) Error: Assertion failed: reentry detected, value=Vector2(-0.0050000000000003375, 0.495), oldValue=Vector2(-0.005, 0.495) at window.assertions.assertFunction (http://localhost/assert/js/assert.js:28:13) at assert (ReadOnlyProperty.ts:325:14) at _notifyListeners (ReadOnlyProperty.ts:276:13) at unguardedSet (ReadOnlyProperty.ts:260:11) at set (Property.ts:54:10) at (Ball.js:255:31) at dragToPosition (BallNode.js:207:13) at _start (DragListener.ts:352:26) at callback (PressListener.ts:729:16) at apply (PhetioAction.ts:162:16) coulombs-law Uncaught Error: Assertion failed: toFixedCustom should match regular toFixed, 0.0 !== -0.1 Error: Assertion failed: toFixedCustom should match regular toFixed, 0.0 !== -0.1 at window.assertions.assertFunction (http://localhost/assert/js/assert.js:28:13) at assert (Utils.js:561:14) at toFixed (Utils.js:614:29) at toFixedNumber (ISLCObjectNode.js:127:39) at _a11yMapPDOMValue (AccessibleValueHandler.ts:527:31) at _getMappedValue (AccessibleValueHandler.ts:383:31) at listener (ReadOnlyProperty.ts:425:4) at link (AccessibleValueHandler.ts:395:33) at (AccessibleSlider.ts:72:6) at (ISLCObjectNode.js:168:4) fourier-making-waves Uncaught Error: Assertion failed: toFixedCustom should match regular toFixed, -0.2 !== -0.3 Error: Assertion failed: toFixedCustom should match regular toFixed, -0.2 !== -0.3 at window.assertions.assertFunction (http://localhost/assert/js/assert.js:28:13) at assert (Utils.js:561:14) at toFixed (TickLabelSet.ts:75:56) at createLabel (TickLabelSet.ts:152:46) at callback (ChartTransform.ts:107:6) at forEachSpacing (TickLabelSet.ts:135:24) at update (TickLabelSet.ts:122:11) at setSpacing (DomainChartNode.ts:242:20) at callback (Multilink.ts:128:6) at (Multilink.ts:184:11) gas-properties Uncaught Error: Assertion failed: openingLeft 3000 must be <= openingRight -2000 Error: Assertion failed: openingLeft 3000 must be <= openingRight -2000 at window.assertions.assertFunction (http://localhost/assert/js/assert.js:28:13) at assert (IdealGasLawContainer.ts:230:14) at getOpeningLeft (IdealGasLawContainer.ts:248:55) at getOpeningWidth (IdealGasLawContainer.ts:114:56) at derivation (DerivedProperty.ts:51:11) at getDerivedValue (DerivedProperty.ts:174:17) at listener (TinyEmitter.ts:123:8) at emit (ReadOnlyProperty.ts:329:22) at _notifyListeners (ReadOnlyProperty.ts:276:13) at unguardedSet (ReadOnlyProperty.ts:260:11) gases-intro Uncaught Error: Assertion failed: openingLeft 3000 must be <= openingRight -2000 Error: Assertion failed: openingLeft 3000 must be <= openingRight -2000 at window.assertions.assertFunction (http://localhost/assert/js/assert.js:28:13) at assert (IdealGasLawContainer.ts:230:14) at getOpeningLeft (IdealGasLawContainer.ts:248:55) at getOpeningWidth (IdealGasLawContainer.ts:114:56) at derivation (DerivedProperty.ts:51:11) at getDerivedValue (DerivedProperty.ts:174:17) at listener (TinyEmitter.ts:123:8) at emit (ReadOnlyProperty.ts:329:22) at _notifyListeners (ReadOnlyProperty.ts:276:13) at unguardedSet (ReadOnlyProperty.ts:260:11) gases-intro Uncaught Error: Assertion failed: openingLeft 3000 must be <= openingRight -2000 Error: Assertion failed: openingLeft 3000 must be <= openingRight -2000 at window.assertions.assertFunction (http://localhost/assert/js/assert.js:28:13) at assert (IdealGasLawContainer.ts:230:14) at getOpeningLeft (IdealGasLawContainer.ts:248:55) at getOpeningWidth (IdealGasLawContainer.ts:179:32) at setWidth (IdealGasLawContainer.ts:196:9) at resizeImmediately (ContainerResizeDragListener.ts:47:20) at _dragListener (PressListener.ts:526:9) at call (DragListener.ts:296:35) at apply (PhetioAction.ts:162:16) at execute (DragListener.ts:447:21) graphing-quadratics Uncaught Error: Assertion failed: toFixedCustom should match regular toFixed, -2.87 !== -2.88 Error: Assertion failed: toFixedCustom should match regular toFixed, -2.87 !== -2.88 at window.assertions.assertFunction (http://localhost/assert/js/assert.js:28:13) at assert (Utils.js:561:14) at toFixed (Utils.js:614:29) at toFixedNumber (CoordinatesNode.ts:50:38) at derivation (DerivedProperty.ts:51:11) at getDerivedValue (DerivedProperty.ts:174:17) at listener (TinyEmitter.ts:123:8) at emit (ReadOnlyProperty.ts:329:22) at _notifyListeners (ReadOnlyProperty.ts:276:13) at unguardedSet (ReadOnlyProperty.ts:260:11) gravity-and-orbits Uncaught Error: Assertion failed: toFixedCustom should match regular toFixed, 4.815336871508379e+24 !== 4.81533687150838e+24 Error: Assertion failed: toFixedCustom should match regular toFixed, 4.815336871508379e+24 !== 4.81533687150838e+24 at window.assertions.assertFunction (http://localhost/assert/js/assert.js:28:13) at assert (Utils.js:561:14) at toFixed (Utils.js:614:29) at toFixedNumber (Utils.js:924:17) at roundToInterval (ValueChangeSoundPlayer.ts:52:60) at constrainValue (ValueChangeSoundPlayer.ts:225:39) at playSoundIfThresholdReached (SliderTrack.ts:155:32) at handleTrackEvent (SliderTrack.ts:168:8) at _start (DragListener.ts:352:26) at callback (PressListener.ts:729:16) greenhouse-effect Uncaught Error: Assertion failed: toFixedCustom should match regular toFixed, -273.1 !== -273.2 Error: Assertion failed: toFixedCustom should match regular toFixed, -273.1 !== -273.2 at window.assertions.assertFunction (http://localhost/assert/js/assert.js:28:13) at assert (Utils.js:561:14) at toFixed (NumberDisplay.ts:146:21) at numberFormatter (NumberDisplay.ts:395:18) at valueToString (NumberDisplay.ts:192:13) at derivation (DerivedProperty.ts:51:11) at getDerivedValue (DerivedProperty.ts:105:25) at (NumberDisplay.ts:191:30) at (ThermometerAndReadout.ts:233:24) at createNode (GroupItemOptions.ts:33:16) interaction-dashboard Uncaught Error: Assertion failed: toFixedCustom should match regular toFixed, 4694728.753291830 !== 4694728.753291829 Error: Assertion failed: toFixedCustom should match regular toFixed, 4694728.753291830 !== 4694728.753291829 at window.assertions.assertFunction (http://localhost/assert/js/assert.js:28:13) at assert (Utils.js:561:14) at toFixed (Utils.js:614:29) at toFixedNumber (Utils.js:924:17) at roundToInterval (ValueChangeSoundPlayer.ts:52:60) at constrainValue (ValueChangeSoundPlayer.ts:225:39) at playSoundIfThresholdReached (Slider.ts:251:34) at drag (SliderTrack.ts:172:16) at _dragListener (PressListener.ts:526:9) at call (DragListener.ts:296:35) least-squares-regression Uncaught Error: Assertion failed: toFixedCustom should match regular toFixed, NaN !== 0.0 least-squares-regression Uncaught Error: Assertion failed: reentry detected, value=[object Object], oldValue=[object Object] Error: Assertion failed: toFixedCustom should match regular toFixed, NaN !== 0.0 at window.assertions.assertFunction (http://localhost/assert/js/assert.js:28:13) at assert (Utils.js:561:14) at toFixed (GraphAxesNode.js:285:54) at (GraphAxesNode.js:76:8) at (LeastSquaresRegressionScreenView.js:187:22) at listener (TinyEmitter.ts:123:8) at emit (ReadOnlyProperty.ts:329:22) at _notifyListeners (ReadOnlyProperty.ts:276:13) at unguardedSet (ReadOnlyProperty.ts:260:11) at set (Property.ts:54:10) Error: Assertion failed: reentry detected, value=[object Object], oldValue=[object Object] at window.assertions.assertFunction (http://localhost/assert/js/assert.js:28:13) at assert (ReadOnlyProperty.ts:325:14) at _notifyListeners (ReadOnlyProperty.ts:276:13) at unguardedSet (ReadOnlyProperty.ts:260:11) at set (Property.ts:54:10) at (ComboBoxListBox.ts:113:20) at apply (PhetioAction.ts:162:16) at execute (ComboBoxListBox.ts:135:19) at inputEvent (Input.ts:1907:69) at dispatchToListeners (Input.ts:1947:11) ph-scale Uncaught Error: Assertion failed: toFixedCustom should match regular toFixed, 0.000209999999999999982 !== 0.000209999999999999954 Error: Assertion failed: toFixedCustom should match regular toFixed, 0.000209999999999999982 !== 0.000209999999999999954 at window.assertions.assertFunction (http://localhost/assert/js/assert.js:28:13) at assert (Utils.js:561:14) at toFixed (Utils.js:614:29) at toFixedNumber (Utils.js:924:17) at roundToInterval (GraphIndicatorDragListener.ts:106:32) at doDrag (GraphIndicatorDragListener.ts:67:35) at _dragListener (PressListener.ts:526:9) at call (DragListener.ts:296:35) at apply (PhetioAction.ts:162:16) at execute (DragListener.ts:447:21) xray-diffraction Uncaught Error: Assertion failed: toFixedCustom should match regular toFixed, 0.087266462599716460 !== 0.087266462599716474 Error: Assertion failed: toFixedCustom should match regular toFixed, 0.087266462599716460 !== 0.087266462599716474 at window.assertions.assertFunction (http://localhost/assert/js/assert.js:28:13) at assert (Utils.js:561:14) at toFixed (Utils.js:614:29) at toFixedNumber (Utils.js:924:17) at roundToInterval (NumberControl.ts:262:29) at constrainValue (SliderTrack.ts:150:31) at handleTrackEvent (SliderTrack.ts:168:8) at _start (DragListener.ts:352:26) at callback (PressListener.ts:729:16) at apply (PhetioAction.ts:162:16) ```

failing sims: http://localhost/aqua/fuzz-lightyear/?loadTimeout=30000&testTask=true&ea&audio=disabled&testDuration=10000&brand=phet&fuzz&testSims=coulombs-law,fourier-making-waves,gas-properties,gases-intro,graphing-quadratics,gravity-and-orbits,greenhouse-effect,interaction-dashboard,least-squares-regression,ph-scale,xray-diffraction

[zepumph]

With this patch, we found a couple cases where toFixedPointString does not behave correctly:

1. With scientific notation provided with an e. Like acid-base-soutions:
   • inputs: 1e-7, 10
   • Utils.toFixed: 0.0000001000
   • toFixedPointString: 1e-7.0000000000
2. many 9s in a row that need to round: Like bamboo:
   • inputs: 2.7439999999999998, 7
   • Utils.toFixed: 2.7440000
   • toFixedPointString: 2.74399910
3. just a super buggy confusing case: Like beers-law-lab:
   • inputs: 0.396704, 2
   • Utils.toFixed: 0.40
   • toFixedPointString: 0.310

I'll stop being formal in formatting and put some more here:

``` // template sim: value digits Utils.toFixed toFixedPointString() bending light: 1.0002929999999999 7 1.0002930 1.00029210 blackbody: 0.4996160344827586 3 0.500 0.4910 buoyancy 99.99999999999999 2 100.00 99.910 charges-and-fields 2.169880191815152 3 2.170 2.1610 circuit-construction-kit-black-box-study 1.0999999999999999e-7 1 0.0 1.1 ``` ```diff Subject: [PATCH] use containsKey, https://github.com/phetsims/my-solar-system/issues/318 --- Index: js/Utils.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/Utils.js b/js/Utils.js --- a/js/Utils.js (revision da8f04fb4ba29c460000ffedb9716350896dafa1) +++ b/js/Utils.js (date 1702585802038) @@ -7,6 +7,7 @@ */ import dot from './dot.js'; +import toFixedPointString from './toFixedPointString.js'; import Vector2 from './Vector2.js'; import Vector3 from './Vector3.js'; @@ -553,7 +554,12 @@ toFixed( value, decimalPlaces ) { const multiplier = Math.pow( 10, decimalPlaces ); const newValue = Utils.roundSymmetric( value * multiplier ) / multiplier; - return newValue.toFixed( decimalPlaces ); // eslint-disable-line bad-sim-text + const regularToFixed = newValue.toFixed( decimalPlaces ); // eslint-disable-line bad-sim-text + + const cmToFixed = toFixedPointString( value, decimalPlaces ); + assert && assert( regularToFixed === cmToFixed, 'different from toFixedPointString', value, decimalPlaces, regularToFixed, cmToFixed ); + + return regularToFixed; }, /**

[zepumph]

@samreid and I worked on this this afternoon. We found multiple cases in which toFixedPointString() was not correct. We also noted the cases where the current toFixed() was incorrect.

@samreid found a polyfill on mdn that is working quite well, and is correct for all cases where the two previous implementations were wrong.

As for performance, it calls toString() on the number a couple times, but we are not worried about that. This is especially true because toFixed() is a view-side function that most likely is not being called within an inner loop many times per frame.

A few more things worth noting:

• the current toFixedPointString() has no usages (should have been obvious but @samreid and I didn't realize it).
• there was a case where molarity was passing in a string instead of a number, an assertion caught it (committed above).
• Large numbers seem to be a spot where our current toFixed() is also quite bad and not consistent. I wrote some unit tests to handle this, but GAO is failing the assertion that I mention below because of it.

Here is the implementation ready for commit. Instead of first committing a code change where toFixed() now runs on the new implementation. We would want to start with an assertion that they match. That way we will know that regressions haven't occurred. I also added a few TODOs for final commit.

```diff Subject: [PATCH] assert toFixed gets a number, https://github.com/phetsims/dot/issues/113 --- Index: js/Utils.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/Utils.js b/js/Utils.js --- a/js/Utils.js (revision 3d5f43c8fbf1159dbe412618f2e9c70442aecc8d) +++ b/js/Utils.js (date 1702590282772) @@ -7,6 +7,7 @@ */ import dot from './dot.js'; +import toFixedPointString from './toFixedPointString.js'; import Vector2 from './Vector2.js'; import Vector3 from './Vector3.js'; @@ -546,6 +547,8 @@ * because the spec doesn't specify whether to round or floor. * Rounding is symmetric for positive and negative values, see Utils.roundSymmetric. * + * TODO: replace this implementation with toFixedPointString(), https://github.com/phetsims/dot/issues/113 + * * @param {number} value * @param {number} decimalPlaces * @returns {string} @@ -555,7 +558,12 @@ const multiplier = Math.pow( 10, decimalPlaces ); const newValue = Utils.roundSymmetric( value * multiplier ) / multiplier; - return newValue.toFixed( decimalPlaces ); // eslint-disable-line bad-sim-text + const regularToFixed = newValue.toFixed( decimalPlaces ); // eslint-disable-line bad-sim-text + + const newToFixed = toFixedPointString( value, decimalPlaces ); + assert && assert( regularToFixed === newToFixed, `toFixed(${value},${decimalPlaces}), newToFixed should match regular toFixed, ${newToFixed} !== ${regularToFixed}` ); + + return regularToFixed; }, /** Index: js/toFixedPointStringTests.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/toFixedPointStringTests.js b/js/toFixedPointStringTests.js --- a/js/toFixedPointStringTests.js (revision 3d5f43c8fbf1159dbe412618f2e9c70442aecc8d) +++ b/js/toFixedPointStringTests.js (date 1702592047544) @@ -12,6 +12,18 @@ QUnit.test( 'tests', assert => { + assert.equal( toFixedPointString( Number.POSITIVE_INFINITY, 0 ), 'Infinity' ); + assert.equal( toFixedPointString( Number.POSITIVE_INFINITY, 1 ), 'Infinity' ); + assert.equal( toFixedPointString( Number.POSITIVE_INFINITY, 5 ), 'Infinity' ); + assert.equal( toFixedPointString( Number.NEGATIVE_INFINITY, 0 ), '-Infinity' ); + assert.equal( toFixedPointString( Number.NEGATIVE_INFINITY, 1 ), '-Infinity' ); + assert.equal( toFixedPointString( Number.NEGATIVE_INFINITY, 5 ), '-Infinity' ); + assert.equal( toFixedPointString( NaN, 0 ), 'NaN' ); + assert.equal( toFixedPointString( NaN, 1 ), 'NaN' ); + + assert.equal( toFixedPointString( 35.855, 0 ), '36' ); + assert.equal( toFixedPointString( 35.855, 3 ), '35.855' ); + assert.equal( toFixedPointString( 35.855, 2 ), '35.86' ); assert.equal( toFixedPointString( 35.854, 2 ), '35.85' ); assert.equal( toFixedPointString( -35.855, 2 ), '-35.86' ); @@ -33,4 +45,47 @@ assert.equal( toFixedPointString( 1.46, 0 ), '1' ); assert.equal( toFixedPointString( 1.46, 1 ), '1.5' ); assert.equal( toFixedPointString( 1.44, 1 ), '1.4' ); + + assert.equal( toFixedPointString( 4.577999999999999, 7 ), '4.5780000' ); + assert.equal( toFixedPointString( 0.07957747154594767, 3 ), '0.080' ); + assert.equal( toFixedPointString( 1e-7, 10 ), '0.0000001000' ); + assert.equal( toFixedPointString( 2.7439999999999998, 7 ), '2.7440000' ); + assert.equal( toFixedPointString( 0.396704, 2 ), '0.40' ); + assert.equal( toFixedPointString( 1.0002929999999999, 7 ), '1.0002930' ); + assert.equal( toFixedPointString( 0.4996160344827586, 3 ), '0.500' ); + assert.equal( toFixedPointString( 99.99999999999999, 2 ), '100.00' ); + assert.equal( toFixedPointString( 2.169880191815152, 3 ), '2.170' ); + assert.equal( toFixedPointString( 1.0999999999999999e-7, 1 ), '0.0' ); + assert.equal( toFixedPointString( 3.2303029999999997, 7 ), '3.2303030' ); + assert.equal( toFixedPointString( 0.497625, 2 ), '0.50' ); + assert.equal( toFixedPointString( 2e-12, 12 ), '0.000000000002' ); + assert.equal( toFixedPointString( 6.98910467173495, 1 ), '7.0' ); + assert.equal( toFixedPointString( 8.976212933741225, 1 ), '9.0' ); + assert.equal( toFixedPointString( 2.9985632338511543, 1 ), '3.0' ); + assert.equal( toFixedPointString( -8.951633986928105, 1 ), -'9.0' ); + assert.equal( toFixedPointString( 99.99999999999999, 2 ), '100.00' ); + assert.equal( toFixedPointString( -4.547473508864641e-13, 10 ), '0.0000000000' ); + assert.equal( toFixedPointString( 0.98, 1 ), '1.0' ); + assert.equal( toFixedPointString( 0.2953388796149264, 2 ), '0.30' ); + assert.equal( toFixedPointString( 1.1119839827800002, 4 ), '1.1120' ); + assert.equal( toFixedPointString( 1.0099982756502124, 4 ), '1.0100' ); + assert.equal( toFixedPointString( -1.5, 2 ), '-1.50' ); + + // TODO: Watch out for GAO on this one, new implementation: 8.343899062588772e+24 !== regular: 8.343899062588771e+24, https://github.com/phetsims/dot/issues/113 + // assert.equal( toFixedPointString( 8.343899062588771e+24, 9 ), '?????' ); + // toFixed(1.113774420948007e+25,9), newToFixed should match regular toFixed, 1.113774420948007e+25 !== 1.1137744209480072e+25 + assert.equal( toFixedPointString( 1.113774420948007e+25, 9 ), '1.113774420948007e+25' ); + assert.equal( toFixedPointString( 1.113774420948007e+25, 9 ), '1.113774420948007e+25' ); + + // TODO: review these tests, I'm not sure what is expected here, https://github.com/phetsims/dot/issues/113 + assert.equal( toFixedPointString( 29495969594939, 3 ), '29495969594939.000' ); + assert.equal( toFixedPointString( 29495969594939, 0 ), '29495969594939' ); + assert.equal( toFixedPointString( 294959695949390000000, 3 ), '294959695949390000000.000' ); + assert.equal( toFixedPointString( 294959695949390000000, 0 ), '294959695949390000000' ); + + /* eslint-disable no-loss-of-precision */ + assert.equal( toFixedPointString( 29495969594939543543543, 0 ), '29495969594939543543543' ); + assert.equal( toFixedPointString( 29495969594939543543543, 0 ), '2.9495969594939544e+22' ); + assert.equal( toFixedPointString( 29495969594939543543543, 4 ), '2.9495969594939544e+22' ); + /* eslint-enable no-loss-of-precision */ } ); \ No newline at end of file Index: js/toFixedPointString.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/toFixedPointString.ts b/js/toFixedPointString.ts --- a/js/toFixedPointString.ts (revision 3d5f43c8fbf1159dbe412618f2e9c70442aecc8d) +++ b/js/toFixedPointString.ts (date 1702590268646) @@ -1,83 +1,52 @@ // Copyright 2022-2023, University of Colorado Boulder /** - * toFixedPointString is a version of Number.toFixed that avoids rounding problems and floating-point errors that exist - * in Number.toFixed and phet.dot.Utils.toFixed. It converts a number of a string, then modifies that string based on the - * number of decimal places desired. It performs symmetric rounding based on only the 2 specific digits that should be - * considered when rounding. Values that are not finite are converted using Number.toFixed. + * TODO: doc, https://github.com/phetsims/dot/issues/113 * - * See https://github.com/phetsims/dot/issues/113 + * TODO: rename this file to `toFixed()`, https://github.com/phetsims/dot/issues/113 * * @author Chris Malley (PixelZoom, Inc.) + * @author Sam Reid (PhET Interactive Simulations) + * @author Michael Kauzmann (PhET Interactive Simulations) */ import dot from './dot.js'; +import Utils from './Utils.js'; function toFixedPointString( value: number, decimalPlaces: number ): string { - assert && assert( isFinite( decimalPlaces ) && decimalPlaces >= 0 ); - - // If value is not finite, then delegate to Number.toFixed and return immediately. - if ( !isFinite( value ) ) { - return value.toFixed( decimalPlaces ); // eslint-disable-line bad-sim-text - } + // eslint-disable-next-line bad-sim-text + return decimalAdjustRound( value, -decimalPlaces ).toFixed( decimalPlaces ); +} - // Convert the value to a string. - let stringValue = value.toString(); +/** + * Decimal adjustment of a number to account for symmetrical rounding and scientific notation. + * Based on code found in https://stackoverflow.com/questions/42109818/reliable-js-rounding-numbers-with-tofixed2-of-a-3-decimal-number + */ +function decimalAdjustRound( value: number, exp: number ): number { - // Find the decimal point in the string. - const decimalPointIndex = stringValue.indexOf( '.' ); - - // If there is a decimal point... - if ( decimalPointIndex !== -1 ) { - - const actualDecimalPlaces = stringValue.length - decimalPointIndex - 1; - if ( actualDecimalPlaces < decimalPlaces ) { - - // There are not enough decimal places, so pad with 0's to the right of decimal point - for ( let i = 0; i < decimalPlaces - actualDecimalPlaces; i++ ) { - stringValue += '0'; - } - } - else if ( actualDecimalPlaces > decimalPlaces ) { + // If the exp is zero, then we can just round the provided value. + if ( exp === 0 ) { + return Utils.roundSymmetric( value ); + } - // There are too many decimal places, so round symmetric. - - // Save the digit that is to the right of the last digit that we'll be saving. - // It determines whether we round up the last digit. - const nextDigit = Number( stringValue[ decimalPointIndex + decimalPlaces + 1 ] ); - - // Chop off everything to the right of the last digit. - stringValue = stringValue.substring( 0, stringValue.length - ( actualDecimalPlaces - decimalPlaces ) ); - - // If we chopped off all of the decimal places, remove the decimal point too. - if ( stringValue.endsWith( '.' ) ) { - stringValue = stringValue.substring( 0, stringValue.length - 1 ); - } + else if ( !isFinite( value ) ) { + return value; + } - // Round up the last digit. - if ( nextDigit >= 5 ) { - const lastDecimal = Number( stringValue[ stringValue.length - 1 ] ) + 1; - stringValue = stringValue.replace( /.$/, lastDecimal.toString() ); - } - } + // If the value is not a number or the exp is not an integer... + else if ( isNaN( value ) || !( exp % 1 === 0 ) ) { + return NaN; } else { - // There is no decimal point, add it and pad with zeros. - if ( decimalPlaces > 0 ) { - stringValue += '.'; - for ( let i = 0; i < decimalPlaces; i++ ) { - stringValue += '0'; - } - } - } + // Shift + const valueStringParts = value.toString().split( 'e' ); + const rounded = Utils.roundSymmetric( +( valueStringParts[ 0 ] + 'e' + ( valueStringParts[ 1 ] ? ( +valueStringParts[ 1 ] - exp ) : -exp ) ) ); - // Remove negative sign from -0 values. - if ( Number( stringValue ) === 0 && stringValue.startsWith( '-' ) ) { - stringValue = stringValue.substring( 1, stringValue.length ); + // Shift back + const roundedStringParts = rounded.toString().split( 'e' ); + return +( roundedStringParts[ 0 ] + 'e' + ( roundedStringParts[ 1 ] ? ( +roundedStringParts[ 1 ] + exp ) : exp ) ); } - - return stringValue; } dot.register( 'toFixedPointString', toFixedPointString );

[pixelzoom]

Search the PhET code base for "trailing zeros" and you'll find numerous places where Utils.toFixedNumber is used so that trailing zeros are removed. Conversely, there are many places where I (and others?) have used Utils.toFixed and I'm relying on the fact that it does NOT remove trailing zeros. So please verify that the solution here does not change the behavior of these 2 functions wrt trailing zeros.

[samreid]

With the patch above, I have these differences between legacy and proposed:

collision-lab Uncaught Error: Assertion failed: toFixed(1.275,2), proposedResult should match legacyResult, 1.28 !== 1.27

This new value seems preferable

gravity-and-orbits Uncaught Error: Assertion failed: toFixed(4.2052273743016777e+24,9), proposedResult should match legacyResult, 4.205227374301678e+24 !== 4.2052273743016777e+24

This is in the noise and I trust the proposed algorithm to be more accurate or acceptable.

hookes-law Uncaught Error: Assertion failed: toFixed(1.7814999999999999,3), proposedResult should match legacyResult, 1.782 !== 1.781

This seems incorrect, probably due to rounding error. Requires investigation.

least-squares-regression Uncaught Error: Assertion failed: toFixed(0,1.3010299956639813), proposedResult should match legacyResult, NaN !== 0.0

This seems incorrect. Investigation reveals the new implementation doesn't support numbers beginning with 0e, but it can be solved with using Number.parseFloat instead of +(number).

ph-scale Uncaught Error: Assertion failed: toFixed(0.00010999999999999999,21), proposedResult should match legacyResult, 0.000109999999999999977 !== 0.000109999999999999990

The legacy result seems more accurate here.

For reference: http://localhost/aqua/fuzz-lightyear/?loadTimeout=30000&testTask=true&ea&audio=disabled&testDuration=10000&brand=phet&fuzz&testSims=collision-lab,gas-properties,gases-intro,geometric-optics-basics,gravity-and-orbits,hookes-law,least-squares-regression,ph-scale          

Here is the patch with Number.parseFloat. @zepumph it still seems there are 2 undesirable results, how should we proceed?

```diff Subject: [PATCH] Update documentation, see https://github.com/phetsims/projectile-data-lab/issues/7 --- Index: js/toFixedPointStringTests.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/toFixedPointStringTests.js b/js/toFixedPointStringTests.js --- a/js/toFixedPointStringTests.js (revision da8f04fb4ba29c460000ffedb9716350896dafa1) +++ b/js/toFixedPointStringTests.js (date 1702607290691) @@ -12,6 +12,18 @@ QUnit.test( 'tests', assert => { + assert.equal( toFixedPointString( Number.POSITIVE_INFINITY, 0 ), 'Infinity' ); + assert.equal( toFixedPointString( Number.POSITIVE_INFINITY, 1 ), 'Infinity' ); + assert.equal( toFixedPointString( Number.POSITIVE_INFINITY, 5 ), 'Infinity' ); + assert.equal( toFixedPointString( Number.NEGATIVE_INFINITY, 0 ), '-Infinity' ); + assert.equal( toFixedPointString( Number.NEGATIVE_INFINITY, 1 ), '-Infinity' ); + assert.equal( toFixedPointString( Number.NEGATIVE_INFINITY, 5 ), '-Infinity' ); + assert.equal( toFixedPointString( NaN, 0 ), 'NaN' ); + assert.equal( toFixedPointString( NaN, 1 ), 'NaN' ); + + assert.equal( toFixedPointString( 35.855, 0 ), '36' ); + assert.equal( toFixedPointString( 35.855, 3 ), '35.855' ); + assert.equal( toFixedPointString( 35.855, 2 ), '35.86' ); assert.equal( toFixedPointString( 35.854, 2 ), '35.85' ); assert.equal( toFixedPointString( -35.855, 2 ), '-35.86' ); @@ -33,4 +45,49 @@ assert.equal( toFixedPointString( 1.46, 0 ), '1' ); assert.equal( toFixedPointString( 1.46, 1 ), '1.5' ); assert.equal( toFixedPointString( 1.44, 1 ), '1.4' ); + + assert.equal( toFixedPointString( 4.577999999999999, 7 ), '4.5780000' ); + assert.equal( toFixedPointString( 0.07957747154594767, 3 ), '0.080' ); + assert.equal( toFixedPointString( 1e-7, 10 ), '0.0000001000' ); + assert.equal( toFixedPointString( 2.7439999999999998, 7 ), '2.7440000' ); + assert.equal( toFixedPointString( 0.396704, 2 ), '0.40' ); + assert.equal( toFixedPointString( 1.0002929999999999, 7 ), '1.0002930' ); + assert.equal( toFixedPointString( 0.4996160344827586, 3 ), '0.500' ); + assert.equal( toFixedPointString( 99.99999999999999, 2 ), '100.00' ); + assert.equal( toFixedPointString( 2.169880191815152, 3 ), '2.170' ); + assert.equal( toFixedPointString( 1.0999999999999999e-7, 1 ), '0.0' ); + assert.equal( toFixedPointString( 3.2303029999999997, 7 ), '3.2303030' ); + assert.equal( toFixedPointString( 0.497625, 2 ), '0.50' ); + assert.equal( toFixedPointString( 2e-12, 12 ), '0.000000000002' ); + assert.equal( toFixedPointString( 6.98910467173495, 1 ), '7.0' ); + assert.equal( toFixedPointString( 8.976212933741225, 1 ), '9.0' ); + assert.equal( toFixedPointString( 2.9985632338511543, 1 ), '3.0' ); + assert.equal( toFixedPointString( -8.951633986928105, 1 ), -'9.0' ); + assert.equal( toFixedPointString( 99.99999999999999, 2 ), '100.00' ); + assert.equal( toFixedPointString( -4.547473508864641e-13, 10 ), '0.0000000000' ); + assert.equal( toFixedPointString( 0.98, 1 ), '1.0' ); + assert.equal( toFixedPointString( 0.2953388796149264, 2 ), '0.30' ); + assert.equal( toFixedPointString( 1.1119839827800002, 4 ), '1.1120' ); + assert.equal( toFixedPointString( 1.0099982756502124, 4 ), '1.0100' ); + assert.equal( toFixedPointString( -1.5, 2 ), '-1.50' ); + + // TODO: Watch out for GAO on this one, new implementation: 8.343899062588772e+24 !== regular: 8.343899062588771e+24, https://github.com/phetsims/dot/issues/113 + // assert.equal( toFixedPointString( 8.343899062588771e+24, 9 ), '?????' ); + // toFixed(1.113774420948007e+25,9), newToFixed should match regular toFixed, 1.113774420948007e+25 !== 1.1137744209480072e+25 + assert.equal( toFixedPointString( 1.113774420948007e+25, 9 ), '1.113774420948007e+25' ); + assert.equal( toFixedPointString( 1.113774420948007e+25, 9 ), '1.113774420948007e+25' ); + + // TODO: review these tests, I'm not sure what is expected here, https://github.com/phetsims/dot/issues/113 + assert.equal( toFixedPointString( 29495969594939, 3 ), '29495969594939.000' ); + assert.equal( toFixedPointString( 29495969594939, 0 ), '29495969594939' ); + // assert.equal( toFixedPointString( 294959695949390000000, 3 ), '294959695949390000000.000' ); + // assert.equal( toFixedPointString( 294959695949390000000, 0 ), '294959695949390000000' ); + // + /* eslint-disable no-loss-of-precision */ + // assert.equal( toFixedPointString( 29495969594939543543543, 0 ), '29495969594939543543543' ); + // assert.equal( toFixedPointString( 29495969594939543543543, 0 ), '2.9495969594939544e+22' ); + // assert.equal( toFixedPointString( 29495969594939543543543, 4 ), '2.9495969594939544e+22' ); + /* eslint-enable no-loss-of-precision */ + + assert.equal( toFixedPointString( 0, 1.3010299956639813 ), '0.0' ); } ); \ No newline at end of file Index: js/Utils.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/Utils.js b/js/Utils.js --- a/js/Utils.js (revision da8f04fb4ba29c460000ffedb9716350896dafa1) +++ b/js/Utils.js (date 1702593926664) @@ -7,6 +7,7 @@ */ import dot from './dot.js'; +import toFixedPointString from './toFixedPointString.js'; import Vector2 from './Vector2.js'; import Vector3 from './Vector3.js'; @@ -546,6 +547,8 @@ * because the spec doesn't specify whether to round or floor. * Rounding is symmetric for positive and negative values, see Utils.roundSymmetric. * + * TODO: replace this implementation with toFixedPointString(), https://github.com/phetsims/dot/issues/113 + * * @param {number} value * @param {number} decimalPlaces * @returns {string} @@ -553,7 +556,12 @@ toFixed( value, decimalPlaces ) { const multiplier = Math.pow( 10, decimalPlaces ); const newValue = Utils.roundSymmetric( value * multiplier ) / multiplier; - return newValue.toFixed( decimalPlaces ); // eslint-disable-line bad-sim-text + const legacyResult = newValue.toFixed( decimalPlaces ); // eslint-disable-line bad-sim-text + + const proposedResult = toFixedPointString( value, decimalPlaces ); + assert && assert( legacyResult === proposedResult, `toFixed(${value},${decimalPlaces}), proposedResult should match legacyResult, ${proposedResult} !== ${legacyResult}` ); + + return legacyResult; }, /** Index: js/toFixedPointString.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/toFixedPointString.ts b/js/toFixedPointString.ts --- a/js/toFixedPointString.ts (revision da8f04fb4ba29c460000ffedb9716350896dafa1) +++ b/js/toFixedPointString.ts (date 1702607272984) @@ -1,84 +1,54 @@ // Copyright 2022-2023, University of Colorado Boulder /** - * toFixedPointString is a version of Number.toFixed that avoids rounding problems and floating-point errors that exist - * in Number.toFixed and phet.dot.Utils.toFixed. It converts a number of a string, then modifies that string based on the - * number of decimal places desired. It performs symmetric rounding based on only the 2 specific digits that should be - * considered when rounding. Values that are not finite are converted using Number.toFixed. + * TODO: doc, https://github.com/phetsims/dot/issues/113 * - * See https://github.com/phetsims/dot/issues/113 + * TODO: rename this file to `toFixed()`, https://github.com/phetsims/dot/issues/113 * * @author Chris Malley (PixelZoom, Inc.) + * @author Sam Reid (PhET Interactive Simulations) + * @author Michael Kauzmann (PhET Interactive Simulations) */ import dot from './dot.js'; - -function toFixedPointString( value: number, decimalPlaces: number ): string { - assert && assert( isFinite( decimalPlaces ) && decimalPlaces >= 0 ); +import Utils from './Utils.js'; - // If value is not finite, then delegate to Number.toFixed and return immediately. - if ( !isFinite( value ) ) { - return value.toFixed( decimalPlaces ); // eslint-disable-line bad-sim-text - } +export default function toFixedPointString( value: number, decimalPlaces: number ): string { + // eslint-disable-next-line bad-sim-text + return decimalAdjustRound( value, -decimalPlaces ).toFixed( decimalPlaces ); +} - // Convert the value to a string. - let stringValue = value.toString(); +/** + * Decimal adjustment of a number to account for symmetrical rounding and scientific notation. + * Based on code found in https://stackoverflow.com/questions/42109818/reliable-js-rounding-numbers-with-tofixed2-of-a-3-decimal-number + */ +function decimalAdjustRound( value: number, exp: number ): number { - // Find the decimal point in the string. - const decimalPointIndex = stringValue.indexOf( '.' ); - - // If there is a decimal point... - if ( decimalPointIndex !== -1 ) { - - const actualDecimalPlaces = stringValue.length - decimalPointIndex - 1; - if ( actualDecimalPlaces < decimalPlaces ) { - - // There are not enough decimal places, so pad with 0's to the right of decimal point - for ( let i = 0; i < decimalPlaces - actualDecimalPlaces; i++ ) { - stringValue += '0'; - } - } - else if ( actualDecimalPlaces > decimalPlaces ) { + // If the exp is zero, then we can just round the provided value. + if ( exp === 0 ) { + return Utils.roundSymmetric( value ); + } - // There are too many decimal places, so round symmetric. - - // Save the digit that is to the right of the last digit that we'll be saving. - // It determines whether we round up the last digit. - const nextDigit = Number( stringValue[ decimalPointIndex + decimalPlaces + 1 ] ); - - // Chop off everything to the right of the last digit. - stringValue = stringValue.substring( 0, stringValue.length - ( actualDecimalPlaces - decimalPlaces ) ); - - // If we chopped off all of the decimal places, remove the decimal point too. - if ( stringValue.endsWith( '.' ) ) { - stringValue = stringValue.substring( 0, stringValue.length - 1 ); - } + else if ( !isFinite( value ) ) { + return value; + } - // Round up the last digit. - if ( nextDigit >= 5 ) { - const lastDecimal = Number( stringValue[ stringValue.length - 1 ] ) + 1; - stringValue = stringValue.replace( /.$/, lastDecimal.toString() ); - } - } + // If the value is not a number... + else if ( isNaN( value ) ) { + return NaN; } else { - // There is no decimal point, add it and pad with zeros. - if ( decimalPlaces > 0 ) { - stringValue += '.'; - for ( let i = 0; i < decimalPlaces; i++ ) { - stringValue += '0'; - } - } - } + // Shift + const valueStringParts = value.toString().split( 'e' ); + const newExponent = valueStringParts[ 1 ] ? ( +valueStringParts[ 1 ] - exp ) : -exp; + const rounded = Utils.roundSymmetric( Number.parseFloat( valueStringParts[ 0 ] + 'e' + newExponent ) ); - // Remove negative sign from -0 values. - if ( Number( stringValue ) === 0 && stringValue.startsWith( '-' ) ) { - stringValue = stringValue.substring( 1, stringValue.length ); + // Shift back + const roundedStringParts = rounded.toString().split( 'e' ); + const roundedExponent = roundedStringParts[ 1 ] ? ( +roundedStringParts[ 1 ] + exp ) : exp; + return Number.parseFloat( roundedStringParts[ 0 ] + 'e' + roundedExponent ); } - - return stringValue; } dot.register( 'toFixedPointString', toFixedPointString ); -export default toFixedPointString; ```

[samreid]

I tried decimal.js and saw it give correct results for all unit tests. The implementation looks like:

export default function toFixedPointString( value, decimalPlaces ) {
  // let time = Date.now();
  let result = new Decimal( value ).toFixed( decimalPlaces, Decimal.ROUND_HALF_UP );

  // Construct the string for '-0.00...' based on decimalPlaces
  const negativeZero = '-0.' + '0'.repeat( decimalPlaces );

  // Check for negative zero and convert it to positive zero
  if ( result === negativeZero ) {
    result = '0.' + '0'.repeat( decimalPlaces );
  }
  // const newDate = Date.now();
  // console.log( 'elapsed time for toFixedPointString', newDate - time, 'ms' );

  return result;
}

let t = Date.now();
for (let i=0;i<1000000;i++){
  toFixedPointString( Math.random(), 2 );
}
let t2 = Date.now();
console.log( 'elapsed time for toFixedPointString', t2 - t, 'ms' );

And on my macbook air m1, it ran 1,000,000 computations in 990ms. That is 0.001ms per computation. May generate some garbage. May be slower on chromebooks or ipads.

Maybe also check big.js if we want a smaller payload.

[zepumph]

How hard would it be to just use the big number library just for the implementation of toFixed. I'm very interested in having that at our disposal as we encounter these sorts of issues that have just already been solved a thousand times.

[samreid]

This patch is working nicely on my working copy:

```diff Subject: [PATCH] https://github.com/phetsims/dot/issues/113 --- Index: chipper/js/common/Transpiler.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/chipper/js/common/Transpiler.js b/chipper/js/common/Transpiler.js --- a/chipper/js/common/Transpiler.js (revision 9fedd6ff6d70cb4b0250c44f32a93666de6b9aeb) +++ b/chipper/js/common/Transpiler.js (date 1703255757077) @@ -104,7 +104,10 @@ const suffix = relativePath.substring( relativePath.lastIndexOf( '.' ) ); // Note: When we upgrade to Node 16, this may no longer be necessary, see https://github.com/phetsims/chipper/issues/1272#issuecomment-1222574593 - const extension = relativePath.includes( 'phet-build-script' ) ? '.mjs' : '.js'; + const extension = ( + relativePath.includes( 'phet-build-script' ) || + relativePath.endsWith( 'big.mjs' ) + ) ? '.mjs' : '.js'; return Transpiler.join( root, 'chipper', 'dist', 'js', ...relativePath.split( path.sep ) ).split( suffix ).join( extension ); } @@ -212,7 +215,13 @@ * @private */ visitFile( filePath ) { - if ( ( filePath.endsWith( '.js' ) || filePath.endsWith( '.ts' ) || filePath.endsWith( '.tsx' ) || filePath.endsWith( '.wgsl' ) ) && !this.isPathIgnored( filePath ) ) { + if ( ( + filePath.endsWith( '.js' ) || + filePath.endsWith( '.ts' ) || + filePath.endsWith( '.tsx' ) || + filePath.endsWith( '.wgsl' ) || + filePath.endsWith( '.mjs' ) + ) && !this.isPathIgnored( filePath ) ) { const changeDetectedTime = Date.now(); const text = fs.readFileSync( filePath, 'utf-8' ); const hash = crypto.createHash( 'md5' ).update( text ).digest( 'hex' ); @@ -317,6 +326,7 @@ // Our sims load this as a module rather than a preload, so we must transpile it this.visitFile( Transpiler.join( '..', repo, 'lib', 'game-up-camera-1.0.0.js' ) ); this.visitFile( Transpiler.join( '..', repo, 'lib', 'pako-2.0.3.min.js' ) ); // used for phet-io-wrappers tests + this.visitFile( Transpiler.join( '..', repo, 'lib', 'big.js-6.2.1', 'big.mjs' ) ); // used for phet-io-wrappers tests Object.keys( webpackGlobalLibraries ).forEach( key => { const libraryFilePath = webpackGlobalLibraries[ key ]; this.visitFile( Transpiler.join( '..', ...libraryFilePath.split( '/' ) ) ); Index: dot/js/Utils.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/dot/js/Utils.js b/dot/js/Utils.js --- a/dot/js/Utils.js (revision 3d5f43c8fbf1159dbe412618f2e9c70442aecc8d) +++ b/dot/js/Utils.js (date 1703260975824) @@ -10,6 +10,9 @@ import Vector2 from './Vector2.js'; import Vector3 from './Vector3.js'; +// eslint-disable-next-line default-import-match-filename +import Big from '../../sherpa/lib/big.js-6.2.1/big.mjs'; + // constants const EPSILON = Number.MIN_VALUE; const TWO_PI = 2 * Math.PI; @@ -552,10 +555,20 @@ */ toFixed( value, decimalPlaces ) { assert && assert( typeof value === 'number' ); + if ( isNaN( value ) ) { + return 'NaN'; + } - const multiplier = Math.pow( 10, decimalPlaces ); - const newValue = Utils.roundSymmetric( value * multiplier ) / multiplier; - return newValue.toFixed( decimalPlaces ); // eslint-disable-line bad-sim-text + // eslint-disable-next-line bad-sim-text + const result = new Big( value ).toFixed( decimalPlaces ); + + // Avoid reporting -0.000 + if ( result.startsWith( '-0.' ) && Number.parseFloat( result ) === 0 ) { + return '0' + result.slice( 2 ); + } + else { + return result; + } }, /** Index: dot/js/toFixedPointString.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/dot/js/toFixedPointString.ts b/dot/js/toFixedPointString.ts --- a/dot/js/toFixedPointString.ts (revision 3d5f43c8fbf1159dbe412618f2e9c70442aecc8d) +++ b/dot/js/toFixedPointString.ts (date 1703255428193) @@ -12,73 +12,12 @@ */ import dot from './dot.js'; +import Utils from './Utils.js'; -function toFixedPointString( value: number, decimalPlaces: number ): string { +export default function toFixedPointString( value: number, decimalPlaces: number ): string { assert && assert( isFinite( decimalPlaces ) && decimalPlaces >= 0 ); - // If value is not finite, then delegate to Number.toFixed and return immediately. - if ( !isFinite( value ) ) { - return value.toFixed( decimalPlaces ); // eslint-disable-line bad-sim-text - } - - // Convert the value to a string. - let stringValue = value.toString(); - - // Find the decimal point in the string. - const decimalPointIndex = stringValue.indexOf( '.' ); - - // If there is a decimal point... - if ( decimalPointIndex !== -1 ) { - - const actualDecimalPlaces = stringValue.length - decimalPointIndex - 1; - if ( actualDecimalPlaces < decimalPlaces ) { - - // There are not enough decimal places, so pad with 0's to the right of decimal point - for ( let i = 0; i < decimalPlaces - actualDecimalPlaces; i++ ) { - stringValue += '0'; - } - } - else if ( actualDecimalPlaces > decimalPlaces ) { - - // There are too many decimal places, so round symmetric. - - // Save the digit that is to the right of the last digit that we'll be saving. - // It determines whether we round up the last digit. - const nextDigit = Number( stringValue[ decimalPointIndex + decimalPlaces + 1 ] ); - - // Chop off everything to the right of the last digit. - stringValue = stringValue.substring( 0, stringValue.length - ( actualDecimalPlaces - decimalPlaces ) ); - - // If we chopped off all of the decimal places, remove the decimal point too. - if ( stringValue.endsWith( '.' ) ) { - stringValue = stringValue.substring( 0, stringValue.length - 1 ); - } - - // Round up the last digit. - if ( nextDigit >= 5 ) { - const lastDecimal = Number( stringValue[ stringValue.length - 1 ] ) + 1; - stringValue = stringValue.replace( /.$/, lastDecimal.toString() ); - } - } - } - else { - - // There is no decimal point, add it and pad with zeros. - if ( decimalPlaces > 0 ) { - stringValue += '.'; - for ( let i = 0; i < decimalPlaces; i++ ) { - stringValue += '0'; - } - } - } - - // Remove negative sign from -0 values. - if ( Number( stringValue ) === 0 && stringValue.startsWith( '-' ) ) { - stringValue = stringValue.substring( 1, stringValue.length ); - } - - return stringValue; + return Utils.toFixed( value, decimalPlaces ); } dot.register( 'toFixedPointString', toFixedPointString ); -export default toFixedPointString; ```

To run it, please apply the patch and unzip big.js-6.2.1 from https://github.com/MikeMcl/big.js/releases/tag/v6.2.1 to sherpa/lib.

Also please restart the transpiler.

If we like this direction, additional facets could be:

1. In transpiler, there is special handling for this file (to be minimal). Should we just treat all *.mjs equally?
2. Move Utils.toFixed out to a separate file so this new dependency only gets imported where needed? (Unless common code uses it, then it would just get imported everywhere anyways).

@zepumph can you please take a look and advise? I'm also happy to check in synchronously if you prefer.

[samreid]

Patch from collaboration with @zepumph

```diff Subject: [PATCH] Do not try to clear , see https://github.com/phetsims/projectile-data-lab/issues/7 --- Index: dot/js/toFixedPointStringTests.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/dot/js/toFixedPointStringTests.js b/dot/js/toFixedPointStringTests.js --- a/dot/js/toFixedPointStringTests.js (revision 3d5f43c8fbf1159dbe412618f2e9c70442aecc8d) +++ b/dot/js/toFixedPointStringTests.js (date 1703272036255) @@ -12,6 +12,18 @@ QUnit.test( 'tests', assert => { + assert.equal( toFixedPointString( Number.POSITIVE_INFINITY, 0 ), 'Infinity' ); + assert.equal( toFixedPointString( Number.POSITIVE_INFINITY, 1 ), 'Infinity' ); + assert.equal( toFixedPointString( Number.POSITIVE_INFINITY, 5 ), 'Infinity' ); + assert.equal( toFixedPointString( Number.NEGATIVE_INFINITY, 0 ), '-Infinity' ); + assert.equal( toFixedPointString( Number.NEGATIVE_INFINITY, 1 ), '-Infinity' ); + assert.equal( toFixedPointString( Number.NEGATIVE_INFINITY, 5 ), '-Infinity' ); + assert.equal( toFixedPointString( NaN, 0 ), 'NaN' ); + assert.equal( toFixedPointString( NaN, 1 ), 'NaN' ); + + assert.equal( toFixedPointString( 35.855, 0 ), '36' ); + assert.equal( toFixedPointString( 35.855, 3 ), '35.855' ); + assert.equal( toFixedPointString( 35.855, 2 ), '35.86' ); assert.equal( toFixedPointString( 35.854, 2 ), '35.85' ); assert.equal( toFixedPointString( -35.855, 2 ), '-35.86' ); @@ -33,4 +45,47 @@ assert.equal( toFixedPointString( 1.46, 0 ), '1' ); assert.equal( toFixedPointString( 1.46, 1 ), '1.5' ); assert.equal( toFixedPointString( 1.44, 1 ), '1.4' ); + + assert.equal( toFixedPointString( 4.577999999999999, 7 ), '4.5780000' ); + assert.equal( toFixedPointString( 0.07957747154594767, 3 ), '0.080' ); + assert.equal( toFixedPointString( 1e-7, 10 ), '0.0000001000' ); + assert.equal( toFixedPointString( 2.7439999999999998, 7 ), '2.7440000' ); + assert.equal( toFixedPointString( 0.396704, 2 ), '0.40' ); + assert.equal( toFixedPointString( 1.0002929999999999, 7 ), '1.0002930' ); + assert.equal( toFixedPointString( 0.4996160344827586, 3 ), '0.500' ); + assert.equal( toFixedPointString( 99.99999999999999, 2 ), '100.00' ); + assert.equal( toFixedPointString( 2.169880191815152, 3 ), '2.170' ); + assert.equal( toFixedPointString( 1.0999999999999999e-7, 1 ), '0.0' ); + assert.equal( toFixedPointString( 3.2303029999999997, 7 ), '3.2303030' ); + assert.equal( toFixedPointString( 0.497625, 2 ), '0.50' ); + assert.equal( toFixedPointString( 2e-12, 12 ), '0.000000000002' ); + assert.equal( toFixedPointString( 6.98910467173495, 1 ), '7.0' ); + assert.equal( toFixedPointString( 8.976212933741225, 1 ), '9.0' ); + assert.equal( toFixedPointString( 2.9985632338511543, 1 ), '3.0' ); + assert.equal( toFixedPointString( -8.951633986928105, 1 ), -'9.0' ); + assert.equal( toFixedPointString( 99.99999999999999, 2 ), '100.00' ); + assert.equal( toFixedPointString( -4.547473508864641e-13, 10 ), '0.0000000000' ); + assert.equal( toFixedPointString( 0.98, 1 ), '1.0' ); + assert.equal( toFixedPointString( 0.2953388796149264, 2 ), '0.30' ); + assert.equal( toFixedPointString( 1.1119839827800002, 4 ), '1.1120' ); + assert.equal( toFixedPointString( 1.0099982756502124, 4 ), '1.0100' ); + assert.equal( toFixedPointString( -1.5, 2 ), '-1.50' ); + + // assert.equal( toFixedPointString( 8.343899062588771e+24, 9 ), '?????' ); + // toFixed(1.113774420948007e+25,9), newToFixed should match regular toFixed, 1.113774420948007e+25 !== 1.1137744209480072e+25 + assert.equal( toFixedPointString( 1.113774420948007e+25, 9 ), '11137744209480070000000000.000000000' ); + + // TODO: review these tests, I'm not sure what is expected here, https://github.com/phetsims/dot/issues/113 + assert.equal( toFixedPointString( 29495969594939, 3 ), '29495969594939.000' ); + assert.equal( toFixedPointString( 29495969594939, 0 ), '29495969594939' ); + // assert.equal( toFixedPointString( 294959695949390000000, 3 ), '294959695949390000000.000' ); + // assert.equal( toFixedPointString( 294959695949390000000, 0 ), '294959695949390000000' ); + // + /* eslint-disable no-loss-of-precision */ + // assert.equal( toFixedPointString( 29495969594939543543543, 0 ), '29495969594939543543543' ); + // assert.equal( toFixedPointString( 29495969594939543543543, 0 ), '2.9495969594939544e+22' ); + // assert.equal( toFixedPointString( 29495969594939543543543, 4 ), '2.9495969594939544e+22' ); + /* eslint-enable no-loss-of-precision */ + + assert.equal( toFixedPointString( 0, 1.3010299956639813 ), '0.0' ); } ); \ No newline at end of file Index: chipper/js/common/Transpiler.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/chipper/js/common/Transpiler.js b/chipper/js/common/Transpiler.js --- a/chipper/js/common/Transpiler.js (revision 9fedd6ff6d70cb4b0250c44f32a93666de6b9aeb) +++ b/chipper/js/common/Transpiler.js (date 1703271509066) @@ -104,7 +104,10 @@ const suffix = relativePath.substring( relativePath.lastIndexOf( '.' ) ); // Note: When we upgrade to Node 16, this may no longer be necessary, see https://github.com/phetsims/chipper/issues/1272#issuecomment-1222574593 - const extension = relativePath.includes( 'phet-build-script' ) ? '.mjs' : '.js'; + const extension = ( + relativePath.includes( 'phet-build-script' ) || + relativePath.endsWith( '.mjs' ) + ) ? '.mjs' : '.js'; return Transpiler.join( root, 'chipper', 'dist', 'js', ...relativePath.split( path.sep ) ).split( suffix ).join( extension ); } @@ -212,7 +215,13 @@ * @private */ visitFile( filePath ) { - if ( ( filePath.endsWith( '.js' ) || filePath.endsWith( '.ts' ) || filePath.endsWith( '.tsx' ) || filePath.endsWith( '.wgsl' ) ) && !this.isPathIgnored( filePath ) ) { + if ( ( + filePath.endsWith( '.js' ) || + filePath.endsWith( '.ts' ) || + filePath.endsWith( '.tsx' ) || + filePath.endsWith( '.wgsl' ) || + filePath.endsWith( '.mjs' ) + ) && !this.isPathIgnored( filePath ) ) { const changeDetectedTime = Date.now(); const text = fs.readFileSync( filePath, 'utf-8' ); const hash = crypto.createHash( 'md5' ).update( text ).digest( 'hex' ); @@ -317,6 +326,7 @@ // Our sims load this as a module rather than a preload, so we must transpile it this.visitFile( Transpiler.join( '..', repo, 'lib', 'game-up-camera-1.0.0.js' ) ); this.visitFile( Transpiler.join( '..', repo, 'lib', 'pako-2.0.3.min.js' ) ); // used for phet-io-wrappers tests + this.visitFile( Transpiler.join( '..', repo, 'lib', 'big.js-6.2.1', 'big.mjs' ) ); // used for phet-io-wrappers tests Object.keys( webpackGlobalLibraries ).forEach( key => { const libraryFilePath = webpackGlobalLibraries[ key ]; this.visitFile( Transpiler.join( '..', ...libraryFilePath.split( '/' ) ) ); Index: dot/js/Utils.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/dot/js/Utils.js b/dot/js/Utils.js --- a/dot/js/Utils.js (revision 3d5f43c8fbf1159dbe412618f2e9c70442aecc8d) +++ b/dot/js/Utils.js (date 1703271911084) @@ -10,6 +10,9 @@ import Vector2 from './Vector2.js'; import Vector3 from './Vector3.js'; +// eslint-disable-next-line default-import-match-filename +import Big from '../../sherpa/lib/big.js-6.2.1/big.mjs'; + // constants const EPSILON = Number.MIN_VALUE; const TWO_PI = 2 * Math.PI; @@ -552,10 +555,26 @@ */ toFixed( value, decimalPlaces ) { assert && assert( typeof value === 'number' ); + if ( isNaN( value ) ) { + return 'NaN'; + } + else if ( value === Number.POSITIVE_INFINITY ) { + return 'Infinity'; + } + else if ( value === Number.NEGATIVE_INFINITY ) { + return '-Infinity'; + } - const multiplier = Math.pow( 10, decimalPlaces ); - const newValue = Utils.roundSymmetric( value * multiplier ) / multiplier; - return newValue.toFixed( decimalPlaces ); // eslint-disable-line bad-sim-text + // eslint-disable-next-line bad-sim-text + const result = new Big( value ).toFixed( decimalPlaces ); + + // Avoid reporting -0.000 + if ( result.startsWith( '-0.' ) && Number.parseFloat( result ) === 0 ) { + return '0' + result.slice( 2 ); + } + else { + return result; + } }, /** Index: dot/js/toFixedPointString.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/dot/js/toFixedPointString.ts b/dot/js/toFixedPointString.ts --- a/dot/js/toFixedPointString.ts (revision 3d5f43c8fbf1159dbe412618f2e9c70442aecc8d) +++ b/dot/js/toFixedPointString.ts (date 1703255428193) @@ -12,73 +12,12 @@ */ import dot from './dot.js'; +import Utils from './Utils.js'; -function toFixedPointString( value: number, decimalPlaces: number ): string { +export default function toFixedPointString( value: number, decimalPlaces: number ): string { assert && assert( isFinite( decimalPlaces ) && decimalPlaces >= 0 ); - // If value is not finite, then delegate to Number.toFixed and return immediately. - if ( !isFinite( value ) ) { - return value.toFixed( decimalPlaces ); // eslint-disable-line bad-sim-text - } - - // Convert the value to a string. - let stringValue = value.toString(); - - // Find the decimal point in the string. - const decimalPointIndex = stringValue.indexOf( '.' ); - - // If there is a decimal point... - if ( decimalPointIndex !== -1 ) { - - const actualDecimalPlaces = stringValue.length - decimalPointIndex - 1; - if ( actualDecimalPlaces < decimalPlaces ) { - - // There are not enough decimal places, so pad with 0's to the right of decimal point - for ( let i = 0; i < decimalPlaces - actualDecimalPlaces; i++ ) { - stringValue += '0'; - } - } - else if ( actualDecimalPlaces > decimalPlaces ) { - - // There are too many decimal places, so round symmetric. - - // Save the digit that is to the right of the last digit that we'll be saving. - // It determines whether we round up the last digit. - const nextDigit = Number( stringValue[ decimalPointIndex + decimalPlaces + 1 ] ); - - // Chop off everything to the right of the last digit. - stringValue = stringValue.substring( 0, stringValue.length - ( actualDecimalPlaces - decimalPlaces ) ); - - // If we chopped off all of the decimal places, remove the decimal point too. - if ( stringValue.endsWith( '.' ) ) { - stringValue = stringValue.substring( 0, stringValue.length - 1 ); - } - - // Round up the last digit. - if ( nextDigit >= 5 ) { - const lastDecimal = Number( stringValue[ stringValue.length - 1 ] ) + 1; - stringValue = stringValue.replace( /.$/, lastDecimal.toString() ); - } - } - } - else { - - // There is no decimal point, add it and pad with zeros. - if ( decimalPlaces > 0 ) { - stringValue += '.'; - for ( let i = 0; i < decimalPlaces; i++ ) { - stringValue += '0'; - } - } - } - - // Remove negative sign from -0 values. - if ( Number( stringValue ) === 0 && stringValue.startsWith( '-' ) ) { - stringValue = stringValue.substring( 1, stringValue.length ); - } - - return stringValue; + return Utils.toFixed( value, decimalPlaces ); } dot.register( 'toFixedPointString', toFixedPointString ); -export default toFixedPointString;

[zepumph]

@samreid and I got to a commit point today. Thanks for the help! I removed toFixedPointString, and updated all unit tests for toFixed(). This is the first time we have imported .mjs with typescript support from a third party, and it is working really well. Webpack didn't complain at all, and furthermore, I like not having the global leaked into the scope. It is only in Utils. This encapsulation is really nice to me.

Search the PhET code base for "trailing zeros" and you'll find numerous places where Utils.toFixedNumber is used so that trailing zeros are removed. Conversely, there are many places where I (and others?) have used Utils.toFixed and I'm relying on the fact that it does NOT remove trailing zeros. So please verify that the solution here does not change the behavior of these 2 functions wrt trailing zeros.

I have confirmed this with unit tests. We have cases for both:

  // Respects only the non zero decimals provided
 assert.equal( Utils.toFixedNumber( 1000.100, 0 ).toString(), '1000' );
  assert.equal( Utils.toFixedNumber( 1000.100, 0 ), 1000 );
  assert.equal( Utils.toFixedNumber( 1000.100, 1 ).toString(), '1000.1' );
  assert.equal( Utils.toFixedNumber( 1000.100, 1 ), 1000.1 );

  // Adds on zeros to make the right number of decimal places
  assert.equal( Utils.toFixed( 29495969594939.1, 3 ), '29495969594939.100' );
  assert.equal( Utils.toFixed( 29495969594939.0, 3 ), '29495969594939.000' );
  assert.equal( Utils.toFixed( 29495969594939., 3 ), '29495969594939.000' );
  assert.equal( Utils.toFixed( 29495969594939, 3 ), '29495969594939.000' );
  assert.equal( Utils.toFixed( 29495969594939, 0 ), '29495969594939' );

I ran performance testing with Big like so, it seems to be about 5x slower. Not sure how much it matters or if we would be able to tell. In my opinion, having consistency in this low level, important feature of phet sims is more important than performance. Furthermore, it is so so sneaky where each of the other implementations fail. It would be nice to just consider this solved. It also fits with existing PhET philosophy in which we would generally rather be complete than focus on optimization.

```diff QUnit.test( 'toFixed performance test', assert => { assert.ok( true ); const t = Date.now(); for ( let i = 0; i < 1000000; i++ ) { Utils.toFixed( Math.random(), 2 ); } const t2 = Date.now(); console.log( 'elapsed time for new toFixed', t2 - t, 'ms' ); const t3 = Date.now(); for ( let i = 0; i < 1000000; i++ ) { Utils.toFixedOld( Math.random(), 2 ); } const t4 = Date.now(); console.log( 'elapsed time for old toFixed', t4 - t3, 'ms' ); /* elapsed time for new toFixed 1024 ms elapsed time for old toFixed 225 ms elapsed time for new toFixed 1475 ms elapsed time for old toFixed 228 ms elapsed time for new toFixed 1446 ms elapsed time for old toFixed 230 ms elapsed time for new toFixed 1443 ms elapsed time for old toFixed 224 ms */ } ); ```

@samreid, what is next here? Please review the commit.

[samreid]

Everything in the commit looks great, thanks! Regarding performance, 1400ms is for calling 1000000 times, so it is just 0.0014ms per call and not something that should cause concern (particularly since this is not often called in an iterative model loop). My only review comment is that WebStorm is showing:

So I'm not sure why it is not a tsc error. I wonder if we just want to get rid of those large numbers?

[zepumph]

I kinda liked having one in that space, just in case Big had an awkward implementation (or at least different) for that class of numbers. Ready to close?

[zepumph]

Looks like serving the mjs file on sparky with nginx is resulting in this error:

Failed to load module script: Expected a JavaScript module script but the server responded with a MIME type of 
"application/octet-stream". Strict MIME type checking is enforced for module scripts per HTML spec.

A google search showed how to solve it: https://kas.kim/blog/failed-to-load-module-script

[zepumph]

Adding our first mjs file has caused some server problems. I had to patch mime types for withServer too. I'll go look for more mime types.

[zepumph]

I didn't find any others, because js isn't supported in this guy:

https://github.com/phetsims/chipper/blob/b4f9348651551c65dbb38900cc59e7e2ecbac5a0/js/common/loadFileAsDataURI.js#L20-L33

[zepumph]

Oh, I'll look into bayes for phettest.

In https://stackoverflow.com/questions/61797225/using-mjs-file-extension-js-modules I found a good solution that worked when I put it at the .htaccess above phettest and deployed dev/rc versions. It should work from here.

[zepumph]

I'm seeing an assertion come in about an undefined for decimal places:

capacitor-lab-basics : fuzz : unbuilt
http://128.138.93.172/continuous-testing/ct-snapshots/1703282283784/capacitor-lab-basics/capacitor-lab-basics_en.html?continuousTest=%7B%22test%22%3A%5B%22capacitor-lab-basics%22%2C%22fuzz%22%2C%22unbuilt%22%5D%2C%22snapshotName%22%3A%22snapshot-1703282283784%22%2C%22timestamp%22%3A1703283021947%7D&brand=phet&ea&fuzz
Query: brand=phet&ea&fuzz
Uncaught Error: Assertion failed: decimal places must be an integer: undefined
Error: Assertion failed: decimal places must be an integer: undefined
at window.assertions.assertFunction (http://128.138.93.172/continuous-testing/ct-snapshots/1703282283784/assert/js/assert.js:28:13)
at assert (Utils.js:556:14)
at toFixed (DragHandleValueNode.js:68:33)
at setValue (DragHandleValueNode.js:55:9)
at (PlateSeparationDragHandleNode.js:65:21)
at (CLBCircuitNode.js:103:42)
at (CapacitanceCircuitNode.js:22:4)
at (CapacitanceScreenView.js:38:34)
at (CapacitanceScreen.js:41:15)
at createView (Screen.ts:307:22)
[URL] http://128.138.93.172/continuous-testing/aqua/html/sim-test.html?url=..%2F..%2Fct-snapshots%2F1703282283784%2Fcapacitor-lab-basics%2Fcapacitor-lab-basics_en.html&simQueryParameters=brand%3Dphet%26ea%2

east-squares-regression : multitouch-fuzz : unbuilt
http://128.138.93.172/continuous-testing/ct-snapshots/1703283437941/least-squares-regression/least-squares-regression_en.html?continuousTest=%7B%22test%22%3A%5B%22least-squares-regression%22%2C%22multitouch-fuzz%22%2C%22unbuilt%22%5D%2C%22snapshotName%22%3A%22snapshot-1703283437941%22%2C%22timestamp%22%3A1703283816403%7D&brand=phet&ea&fuzz&fuzzPointers=2&supportsPanAndZoom=false
Query: brand=phet&ea&fuzz&fuzzPointers=2&supportsPanAndZoom=false
Uncaught Error: Assertion failed: decimal places must be an integer: 1.3010299956639813
Error: Assertion failed: decimal places must be an integer: 1.3010299956639813
at window.assertions.assertFunction (http://128.138.93.172/continuous-testing/ct-snapshots/1703283437941/assert/js/assert.js:28:13)
at assert (Utils.js:556:14)
at toFixed (GraphAxesNode.js:285:54)
at (GraphAxesNode.js:76:8)
at (LeastSquaresRegressionScreenView.js:187:22)
at listener (TinyEmitter.ts:123:8)
at emit (ReadOnlyProperty.ts:329:22)
at _notifyListeners (ReadOnlyProperty.ts:276:13)
at unguardedSet (ReadOnlyProperty.ts:260:11)
at set (Property.ts:54:10)
[URL] http://128.138.93.172/continuous-testing/aqua/html/sim-test.html?url=..%2F..%2Fct-snapshots%2F1703283437941%2Fleast-squares-regression%2Fleast-squares-regression_en.html&simQueryParameters=brand%3Dphet%26ea%26fuzz%26fuzzPointers%3D2%26supportsPanAndZoom%3Dfalse&testInfo=%7B%22test%22%3A%5B%22least-squares-regression%22%2C%22multitouch-fuzz%22%2C%22unbuilt%22%5D%2C%22snapshotName%22%3A%22snapshot-1703283437941%22%2C%22timestamp%22%3A1703283816403%7D
[NAVIGATED] http://128.138.93.172/continuous-testing/aqua/html/sim-test.html?url=..%2F..%2Fct-snapshots%2F1703283437941%2Fleast-squares-regression%2Fleast-squares-regression_en.html&simQueryParameters=brand%3Dphet%26ea%26fuzz%26fuzzPointers%3D2%26supportsPanAndZoom%3Dfalse&testInfo=%7B%22test%22%3A%5B%22least-squares-regression%22%2C%22multitouch-fuzz%22%2C%22unbuilt%22%5D%2C%22snapshotName%22%3A%22snapshot-1703283437941%22%2C%22timestamp%22%3A1703283816403%7D
[NAVIGATED] about:blank
[NAVIGATED] http://128.138.93.172/continuous-testing/ct-snapshots/1703283437941/least-squares-regression/least-squares-regression_en.html?continuousTest=%7B%22test%22%3A%5B%22least-squares-regression%22%2C%22multitouch-fuzz%22%2C%22unbuilt%22%5D%2C%22snapshotName%22%3A%22snapshot-1703283437941%22%2C%22timestamp%22%3A1703283816403%7D&brand=phet&ea&fuzz&fuzzPointers=2&supportsPanAndZoom=false
[CONSOLE] enabling assert
[CONSOLE] continuous-test-load

[zepumph]

Ok, CT is looking clear. @samreid can you please review?

[samreid]

Everything seems great, nice work @zepumph. Closing.

[phet-dev]

Reopening because there is a TODO marked for this issue.

[samreid]

The remaining todo is in ScientificNotationNode:

    if ( options.exponent !== null ) {

      // M x 10^E, where E is options.exponent
      //TODO https://github.com/phetsims/dot/issues/113 division here and in Utils.toFixed can result in floating-point error that affects rounding
      mantissa = Utils.toFixed( value / Math.pow( 10, options.exponent ), options.mantissaDecimalPlaces );
      exponent = options.exponent.toString();
    }

This TODO was added in https://github.com/phetsims/scenery-phet/commit/5a0322532f89448bce100dac91578fb496a03ef4. I also commented on a related TODO in ScientificNotationNode in https://github.com/phetsims/scenery-phet/issues/613

@zepumph and I changed Utils.toFixed to use big.js, which describes itself as "A small, fast JavaScript library for arbitrary-precision decimal arithmetic.". @pixelzoom can you please review the TODOs in ScientificNotationNode and check their status? Please zoom consult with me and/or @zepumph as needed.

[pixelzoom]

@pixelzoom can you please review the TODOs in ScientificNotationNode and check their status?

Can do. But it looks like you've assigned that work to me in https://github.com/phetsims/scenery-phet/issues/613. So I have adjusted the TODO and will close this issue.