fix: progress_table_cell_type floating point rounding issues #2380

[dbranley]

The PR fulfills these requirements: (check all the apply)

• It's submitted to the main branch.
• When resolving a specific issue, it's referenced in the PR's title (e.g. feat: Add a button #xxx, where "xxx" is the issue number).
• When resolving a specific issue, the PR description includes Closes #xxx, where "xxx" is the issue number.
• If changes were made to ui folder, unit tests (make test) still pass.
• New/updated tests are included

Closes #2380

I have fixed this bug relating to floating-point error when rendering the ProgressTableCellType component.

The bug is in progress_table_cell_type.tsx where the progress value is multiplied by 100. Here's a snipit of that:

<div className={css.percent}>{`${Number.isInteger(progress * 1000) ? (progress * 100) : (progress * 100).toFixed(2)}%`}</div>

This bit of code can result in unexpected behavior because of floating-point arithmetic. The value 0.14 is an example of this and when multiplied by 100 it actually returns 14.000000000000002. Here is a screen shot from the Chrome js console where I show a few examples of this happening:

Background: If you recall from the PR I submitted back in May to introduced these decimal values here, there was a discussion about not keeping trailing 0's and instead ensuring the rendered % value had the same number of digits as the original input, with up to 2 decimal places. Here's that discussion from the PR. That's the reason we simply had (progress * 100) without toFixed(2) here.

I found a few ways to resolve this, but I decided the most efficient way to deal with this is to just avoid multiplying by 100 and instead multiply by 1000 then divide that by 10. We don't need this in the else-block since we have the toFixed(2) there. Here's what that looks like:

<div className={css.percent}>{`${Number.isInteger(progress * 1000) ? (progress * 1000)/10 : (progress * 100).toFixed(2)}%`}</div>

If there is a more efficient or elegant way to resolve this, please let me know.

I modified my local copy of the table py example to have values of 0.14 and you can see here it now renders fine:

I also created a new test in progress_table_cell_type_test.tsx that tested 6 different float values that cause this issue. Here's a snipit of the values tested and expected results:

  progressFloatingPointValues = [
    {input: 0.14, output: '14%'},
    {input: 0.148, output: '14.8%'},
    {input: 0.1488, output: '14.88%'},
    {input: 0.29, output: '29%'},
    {input: 0.58, output: '58%'},
    {input: 0.592, output: '59.2%'},
  ]  

I know that I could have just added these values to the progressValues array defined just above and then these would have been tested as part of the existing test Renders data-test attr with decimal values. But I thought it was important for this bug to have its own named test case since it is meant to verify a very specific scenario.

Before fixing the bug, I ran the new test and you can see here it failed as expected:

After the fix is applied, everything works fine. Here's a screen shot after running all test suites:

Let me know if you have any questions or would like me to make any changes. Thanks!

[mturoci]

Thanks! Wondering why does division by 1000 work though?

[dbranley]

Thanks! Wondering why does division by 1000 work though?

@mturoci I assume you meant to say multiplication by 1000? Here are a couple good write-ups about this issue: https://floating-point-gui.de/basic/ and https://0.30000000000000004.com/, but I haven't had the chance to dig too deeply into 'exactly' why this is a problem for 100 and not 1000. But I did create a small test and verify that all 3 digit decimal places from .001 to .999 don't have any issues with multiplication by 1000, so the change should be safe for this use case. In all my years as a developer, I've never encountered this problem. I assume that's because I would always be doing some sort of rounding or truncating of decimals for display purposes - but in this scenario we aren't doing that, as I explained in the original PR comment.

Let me know what you think!

[dbranley]

@mturoci I had some free time this weekend so decided to dig further into the question about why 0.14x100 is a problem but 0.14x1000 is not. But a disclaimer before I start, my math is really, really rusty so I might be interpreting or describing some of this incorrectly...but here goes!

To summarize the root cause of this problem, I'll quote from one of the links I shared above:

...computers use a format (binary floating-point) that cannot accurately represent a number like 0.1, 0.2 or 0.3 at all.

And this includes 0.14. Therefore when you do binary math on these types of numbers, you can't guarantee that you'll end up with a number you would expect. We already know that javascript returns 14.000000000000002 for 0.14x100 and that it returns 140 for 0.14x1000. To help me understand why this is happening, I decided to do the binary math myself to see if I could gleam any insights. The first thing I did was convert 0.14 to binary. I decided to do this by hand since none of the calculators I could fine online would return enough binary digits and I wanted to convert this to 64 binary digits. So the 64 digit binary number I calculated for 0.14 is:

• 0.001000111101011100001010001111010111000010100011110101110000101

For 100 the binary equivalent is 1100100.0, and for 1000 the binary equivalent is 1111101000.0.

I found a good binary calculator online here. You can't include the decimal points, so I need to apply those back after doing the math.

First I started with 0.14x100 which gave me this:

• 011100000000000000000000000000000000000000000000000001000000000000000

After applying the decimal place you have this:

• 01110.0000000000000000000000000000000000000000000000001000000000000000

And when you convert this to decimal you end up with 14.000000000000002, which is the same we get from javascript.

Now when you do the binary math for 0.14x1000 like this you get:

• 0100011000000000000000000000000000000000000000000000000000000000000000000

After applying the decimal place you have this:

• 010001100.0000000000000000000000000000000000000000000000000000000000000000

And when you convert this to decimal you end up with 140

So I think what's happening is that any straggling 1's when multiplying by 1000 are getting pushed out of range for this 64-bit binary number to hang onto, and therefore we end up with only 0's after the decimal and hence it rounds cleanly to 140 as expected. I didn't try it, but I assume if I used a binary number with more than 64 digits, I would eventually end up with 1's somewhere along the way.

Let me know what you think and if this seems reasonable. And of course if you see something I'm overlooking, please let me know. Like I said at the top, my math is very rusty! Thanks!

[mturoci]

Thanks for the summary @dbranley, but I meant something different. I understand what floating-point precision is, my question was more about why it works in our specific usecase and thus whether we can confidently use it as it safely covers all the other edgecases etc.

I had a deeper look and think I found out why it works and why it makes sense to use your fix.

Our formatting logic is as follows:

• All the progress values should range from 0 - 1.
• Check if the number is going to have <= 1 decimal places (by multiplying it by 1000). If so, multiply by 1000 - remove floating point and then divide by 10 to render correct value. Division by 10 is not guaranteed to not yield precision errors, but seems to work just fine.
• If the number will have >= 2 decimal places, use toFixed to limit up to 2 decimal places and call it a day.

This means your solution should cover everything correctly with the only caveat being division by 10. From my quick testing, it looks like it should be fine. In case we find out it's not, we can still special-case it and use toFixed.

[mturoci]

Removed the infamous number from test case. Otherwise LGTM. Thanks @dbranley!

[dbranley]

Thanks @mturoci for the explanation! Yeah I misunderstood your original question, so I appreciate the clarification.

But digging into this was a good math refresher for me, so I'm glad I spent time on it this weekend!