ScientificNotationNode rounds numbers incorrectly

[Luisav1]

The ScientificNotationNode rounds first to the number of decimals specified plus one seen below.

// Convert to a string in exponential notation (eg 2e+2).
// Request an additional decimal place, because toExponential uses toFixed, which doesn't round the same on all platforms
const exponentialString = value.toExponential( options.mantissaDecimalPlaces + 1 );

This causes the number to be rounded up twice in some cases, resulting in incorrect rounding as in the example in the patch below. The number 424.8 rounded to 2 significant digits should be 4.2 x 10^2. Other numbers like this are in this issue phetsims/build-a-nucleus#24.

Index: js/simula-rasa/view/SimulaRasaScreenView.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/simula-rasa/view/SimulaRasaScreenView.ts b/js/simula-rasa/view/SimulaRasaScreenView.ts
--- a/js/simula-rasa/view/SimulaRasaScreenView.ts	(revision 57b1d5c6bc94a6babadb6d5578824d9fcefa50e2)
+++ b/js/simula-rasa/view/SimulaRasaScreenView.ts	(date 1655840680453)
@@ -12,6 +12,8 @@
 import simulaRasa from '../../simulaRasa.js';
 import SimulaRasaModel from '../model/SimulaRasaModel.js';
 import optionize from '../../../../phet-core/js/optionize.js';
+import ScientificNotationNode from '../../../../scenery-phet/js/ScientificNotationNode.js';
+import NumberProperty from '../../../../axon/js/NumberProperty.js';
 
 type SelfOptions = {
  //TODO add options that are specific to SimulaRasaScreenView here
@@ -43,6 +45,9 @@
       tandem: options.tandem.createTandem( 'resetAllButton' )
     } );
     this.addChild( resetAllButton );
+
+    const number = new ScientificNotationNode( new NumberProperty( 424.8 ) );
+    this.addChild( number );
   }
 
   /**

@pixelzoom can you take a look at this and see if there's a way to fix it? Thanks!

[pixelzoom]

I reproduced the problem using the above patch to simula-rasa. To clarify... In the above example, 424.8 should be 4.2 x 10^2, not 4.3 x 10^2. The above patch is displaying 4.3. x 10^2 in SimulaRasaScreenView.

Repos/sims that use ScientificNotationNode include:

• blackbody-spectrum
• build-a-nucleus
• inverse-square-law-common (coulombs-law, gravity-force-lab, gravity-force-lab-basics)
• ph-scale (ph-scale-basics)

[pixelzoom]

The first thing I checked was whether this problem was introduced during TypeScript conversion. So I revert to e0d0199, which is when the file was renamed from .js to .ts, but had not otherwise been changed. The problem was still present in the SimulaRasaScreenView patch, so it would seem that this problem has been around for awhile.

I then isolated the problem to the static toScientificNotation method, which is called by update.

In the brower console:

// mantissa should be 4.2
> phet.sceneryPhet.ScientificNotationNode.toScientificNotation( 424.8 )
{mantissa: '4.3', exponent: '2'}

[pixelzoom]

In the above commits, I improved some documentation.

In 8048792, I did a bit of TypeScript/options cleanup.

[pixelzoom]

Going through the cases in toScientificNotationNode, there are other problems.

The first case is when the value is 0. If the value is zero, the exponent is always set to 1:

    if ( value === 0 ) {
      mantissa = 0;
      exponent = 1;
    }

That's incorrect, as shown in these examples:

// exponent should be 2, because that's what we asked for in options
> phet.sceneryPhet.ScientificNotationNode.toScientificNotation( 0, { exponent: 2 } )
{mantissa: '0.0', exponent: '1'}

// exponent should be 0, because we asked for exponent to be computed
> phet.sceneryPhet.ScientificNotationNode.toScientificNotation( 0, { exponent: null } )
{mantissa: '0.0', exponent: '1'}

Fixed in the above commit, so that the above examples are:

> phet.sceneryPhet.ScientificNotationNode.toScientificNotation( 0, { exponent: 2 } )
{mantissa: '0.0', exponent: '2'}

> phet.sceneryPhet.ScientificNotationNode.toScientificNotation( 0, { exponent: null } )
{mantissa: '0.0', exponent: '0'}

[pixelzoom]

The next case in toScientificNotationNode is:

    else if ( options.exponent !== null && options.exponent === 0 ) {
      mantissa = Utils.toFixedNumber( value, options.mantissaDecimalPlaces );
      exponent = 0;
    }

The if expression here is redundant. We don't need to check options.exponent !== null, because null !== 0.

Fixed in the above commit.

[pixelzoom]

The next case is where the problem that was reported occurs:

    else {

      // Convert to a string in exponential notation (eg 2e+2).
      // Request an additional decimal place, because toExponential uses toFixed, which doesn't round the same on all platforms.
      const exponentialString = value.toExponential( options.mantissaDecimalPlaces + 1 );

We're requesting one extra decimal place, to do our own rounding later, due to problems with built-in toFixed. But the call to toExponential is doing some rounding. So for our value of 424.8:

> var value = 424.8
> value.toExponential(  2 )
'4.25e+2'

Then later in the code, 4.25 will be rounded to 1 decimal place, resulting in 4.3. So yes, we're rounding twice.

This is going to require some serious reimplementation, with requirements:

• round only once
• do not use toFixed, because it rounds differently on different platforms

[pixelzoom]

In the above commits, I added unit tests for toScientificNotation, the part of ScientificNotationNode that's unit-testable.

[pixelzoom]

In the above commit, I added an implementation and unit tests for the case where a non-zero exponent is supplied via options:

    else if ( options.exponent !== null ) {

      // M x 10^E, where E is options.exponent
      mantissa = Utils.toFixedNumber( value / Math.pow( 10, options.exponent ), options.mantissaDecimalPlaces );
      exponent = options.exponent;
    }

In the process, I discovered a problem with dot.Utils.toFixed:

// mantissa should be '35.86'
> phet.sceneryPhet.ScientificNotationNode.toScientificNotation( 35.855, { exponent: 0, mantissaDecimalPlaces: 2 } )
{mantissa: '35.85', exponent: '0'}

// should be '35.86'
> phet.dot.Utils.toFixed( 35.855, 2 )
'35.85'

// should be 3585.5
>  35.855 * 100
3585.4999999999995

35.855 rounded to 2 decimal places should be '35.86', but toFixed returns '35.85'. As you can see by the last line, this is a problem that's inherent in floating-point numbers. And I'm not sure if this is solvable, or worth trying to solve. Tracking in phetsims/dot#113.

[pixelzoom]

In phetsims/dot#113, I wrote toFixedPointString, which doesn't have the rounding and floating-point problems inherent in Number.toFixed and dot.Utils.toFixed. I'm going to investigate reimplementing ScientificNotationNode using that function. It will be a lot of work, and will require a lot of regression testing, hopefully with assistance from QA. I've asked about priority of this work in phetsims/build-a-nucleus#24 (comment).

[pixelzoom]

Summary of above commits:

• Simplified the implementation of toScientificNotation by removing (redundant) code for special cases that are handled by the general cases.
• Eliminated double rounding. The cases reported in Half-life rounding errors build-a-nucleus#24 was working now, and are being unit tested.
• Used parseFloat and parseInt instead of Number(...) to convert strings to numbers.
• Reduced the number of conversions between number and string.
• Added TODO comments to identify problems.

[pixelzoom]

I found myself needing a test harness for ScientificNotationNode. So I created one in scenery-phet demo, screenshot below. I suspect this will be useful for developers and QA too.

[pixelzoom]

In 6/30 dev meeting:

• @kathy-phet said that ScientificNotationNode is OK as is for now, with its known problems, and is not blocking
• We may revisit this when problems with Utils.toFixed are addressed, see Utils.toFixed returns incorrect results in some cases dot#113
• Addressing Utils.toFixed will not address the use of Number.toExpotential (which uses built-in toFixed), and there's not a good replacement for it. We'll live with it for now.
• @pixelzoom will summarize status of this issue, and assign back to @Luisav1 for review.

[pixelzoom]

Summary:

• The "rounding twice" problem has been fixed.
• There are still some numbers that will not be represented as expected, due to the nature of floating-point operations in JavaScript. Specifically:
  • With option exponent: null, ScientificNotationNode is subject to floating-point errors inherent in dot.Utils.toFixed, as described in Utils.toFixed returns incorrect results in some cases dot#113.
  • With options exponent: N, where N is !== 0 and N !== null, ScientificNotationNode is still subject to floating-point errors inherent in Number.toExpotential.
• The decision has been made to investigate addressing floating-point more generally (perhaps via a library like math.js), rather than specifically for ScientificNotationNode.
• ScientificNotationNode now has a manual test harness. See the Components screen of scenery-phet demo.
• ScientificNotationNode.toScientificNotation now has Qunit tests. This set of tests is by no means "complete" and should be expanded as needed. See ScientificNotationNodeTests.js.

Back to @Luisav1 to review. If this meets your needs for build-a-nucleaus, please assign back to me.

[pixelzoom]

@Luisav1 reminder that this issue is assigned to you for review. Let me know if you have questions.

[Luisav1]

Thanks @pixelzoom!

@chrisklus, @marlitas, and I first ran the QUnit tests which all passed. We reviewed the cases set for each test which were working as expected.
We also checked the demo in scenery-phet and checked issue phetsims/build-a-nucleus#24 there and a few other cases, that also rounded the numbers correctly.
Lastly, we reviewed the summary bullet points which seemed reasonable.

This should be good, assigning it back to @pixelzoom.

[pixelzoom]

👍🏻 closing.