Utils.toFixed returns incorrect results in some cases #113
Comments
|
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 I'm planning to use this in |
|
From dev meeting today, we will wait to discuss with @pixelzoom and @jonathanolson. |
pixelzoom
changed the title
|
@liam-mulhall suggested a library like math.js that may have support for handling this. @pixelzoom: Since this is a systemic problem, should anything be done for ScientificNotationNode and build-a-nucleus? @pixelzoom wrote a string manipulation algorithm to get around this in #113 (comment). But he found it couldn't be used for ScientificNotationNode because division was required before the string manipulation. @kathy-phet: Lets bring this up during quarterly goals discussion, this might be a good project to work on generally. |
|
This investigation should be handled over in the epic issue in #116 |
|
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): Note this returns a number instead of a string, so would still need one more step before it could be used for Also, we would want to adapt it to run as a regular function instead of monkey patching Math. |
|
At 7/6/2022 quarterly-planning meeting, this was assigned to me for next steps, starting with the polyfill that @samreid identified in #113 (comment). |
|
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. |
|
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. |
|
@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. |
|
@samreid mentioned it may be pretty straight forward when calling We aren't really sure on the priority though. |
|
I'm testing this patch in aqua: 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' |
|
@samreid and I will discuss the discrepancies that he is finding. |
|
Results of local aqua: |
|
With this patch, we found a couple cases where
I'll stop being formal in formatting and put some more here: 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;
},
/**
|
|
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? |
|
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? |
|
Looks like serving the mjs file on sparky with nginx is resulting in this error: A google search showed how to solve it: https://kas.kim/blog/failed-to-load-module-script |
|
Adding our first |
|
I didn't find any others, because |
|
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. |
|
I'm seeing an assertion come in about an |
|
Ok, CT is looking clear. @samreid can you please review? |
|
Everything seems great, nice work @zepumph. Closing. |
|
Reopening because there is a TODO marked for this issue. |
|
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 phetsims/scenery-phet@5a03225. I also commented on a related TODO in ScientificNotationNode in phetsims/scenery-phet#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. |
Can do. But it looks like you've assigned that work to me in phetsims/scenery-phet#613. So I have adjusted the TODO and will close this issue. |
Discovered while working on phetsims/scenery-phet#747.
Utils.toFixedwas created as a workaround for problems with built-intoFixed, which rounds differently on different browsers.Here's the current implementation:
But
Utils.toFixeddoes not return the correct value in all cases. It's subject to floating-point error.For example, this should be '35.86':
This is caused by
value * multiplierin the implementation ofUtils.toFixed: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?
The text was updated successfully, but these errors were encountered: