Utils.numberOfDecimalPlaces is buggy.

[pixelzoom]

Something is very wrong with numberOfDecimalPlaces.

These are correct:

> phet.dot.Utils.numberOfDecimalPlaces( 0.50 )
1
> phet.dot.Utils.numberOfDecimalPlaces( 0.51 )
2
> phet.dot.Utils.numberOfDecimalPlaces( 0.52 )
2
> phet.dot.Utils.numberOfDecimalPlaces( 0.53 )
2
> > phet.dot.Utils.numberOfDecimalPlaces( 0.54 )
2

Then these are wrong:

> phet.dot.Utils.numberOfDecimalPlaces( 0.55 )
3
> phet.dot.Utils.numberOfDecimalPlaces( 0.56 )
3
> phet.dot.Utils.numberOfDecimalPlaces( 0.57 )
3
> phet.dot.Utils.numberOfDecimalPlaces( 0.58 )
3

Then these are corrrect:

> phet.dot.Utils.numberOfDecimalPlaces( 0.59 )
2
> phet.dot.Utils.numberOfDecimalPlaces( 0.60 )
1

This doesn't happen for anything other than 0.56, 0.57, 0.58. And it's fine for 1.56 etc.

> Utils.numberOfDecimalPlaces( 0.66 )
2
> Utils.numberOfDecimalPlaces( 1.56 )
2

[pixelzoom]

Ugh, there's a floating-point precision problem in this implementation:

  numberOfDecimalPlaces( value ) {
    let count = 0;
    let multiplier = 1;
    while ( ( value * multiplier ) % 1 !== 0 ) {
      count++;
      multiplier *= 10;
    }
    return count;
  },

The second time through the while loop, value * multiplier is 5.6000000000000005 not 5.6.

[pixelzoom]

Doing a Google search, the general advice was to implement this using strings, since trying to do it with numbers introduces floating-point approximation errors at some point. So here's the new implementation, optimized for integers:

  numberOfDecimalPlaces( value ) {
    if ( Math.floor( value ) === value ) {
      return 0;
    }
    else {
      return value.toString().split( '.' )[ 1 ].length;
    }
  },

I added a unit test specifically for the 0.56 case, verify that unit tests are passing.

@jonathanolson would you mind reviewing? High-priority please, because this is so widely used.

[jonathanolson]

Looks workable, but doesn't work for things where the toString() would return scientific notation (e.g. 1e-50). Let's error out in those cases, and properly handle the other ones?

[pixelzoom]

Thanks @jonathanolson, good suggestions. I hadn't consider scientific notation. I've added assertions to fail if it's not a number, not a finite number, or if it's a non-integer in scientific notation. See commit above, and here's the implementation:

  numberOfDecimalPlaces( value ) {
    assert && assert( typeof value === 'number' && isFinite( value ), `value must be a finite number ${value}` );
    if ( Math.floor( value ) === value ) {
      return 0;
    }
    else {
      assert && assert( !value.toString().includes( 'e' ), `scientific notation is not supported: ${value}` );
      return value.toString().split( '.' )[ 1 ].length;
    }
  },

One more round of review please.

[jonathanolson]

Looks great to me, thanks!

[pixelzoom]

Thanks for the review and help. Closing.

[pixelzoom]

Reopening. This is causing at least one sim to fail, see https://github.com/phetsims/ph-scale/issues/233#event-5175701120. I won't be able to investigate until 8/23 at the earliest.

[KatieWoe]

Same issue in https://github.com/phetsims/scenery-phet/issues/696 and https://github.com/phetsims/gene-expression-essentials/issues/139 that I saw

[jonathanolson]

Looking into a generalization that should work.

[jonathanolson]

Patched, added more unit tests for some assorted cases. @pixelzoom can you review? @KatieWoe can you let me know if the failures go away?

[KatieWoe]

It is looking ok in CT. I'll let you know if that changes

[pixelzoom]

@jonathanolson thanks for stepping in to handle this while I was gone.

Implenentation looks nice. Closing.