-
Notifications
You must be signed in to change notification settings - Fork 102
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
NumberFormat may give too much precision for some Numbers #128
Comments
It's not just for large numbers, also for decimals showing up to 20 digits of precision, we may get less precise answers. See the test here: tc39/test262#857 I wonder if the answer here is to have a default maxSignificantDigits settling which reflects the inherent precision of doubles. I can understand one ICU doesn't have this default--the same DecimalFormat class is used for 64-bit ints, which may have more significant figures (maybe we'll extend NumberFormat for a BigInt overload, which could make this idea not make sense, anyway). Or we could leave that setting the same, and just adopt whatever algorithm ICU is using here for finding the string of digits. |
UPDATE: this bug was fixed, seems |
@Yaffle Yikes, that's awful. Seems like an ICU bug, since multiple browsers seem to just call out to ICU for all of this logic and they share the same bug. I reduced the test case to just calling ICU, and filed an issue upstream at http://bugs.icu-project.org/trac/ticket/12989 . |
@littledan , thanks! |
This issue should probably be tagged "Web Reality", but I don't have tagging rights. |
Note: 12344501000000000487815444678311936 === 12344501000000000000000000000000000 for binary64 floating points; Ref tc39/ecma402#128
1) (123.44500) == 123.444999999999993179 2) (123.44500).toPrecision(5) === "123.44" gives correct value in Chrome and Firefox; Ref tc39/ecma402#128
This is shane from the ICU team. This is a known issue. Here's the upstream ticket: http://bugs.icu-project.org/trac/ticket/11318 I'm working on a major overhaul of the number formatting pipeline in ICU, and this is going to be addressed as part of that effort. |
@sffc Great, thanks Shane! Just wondering, when this is done, will it be resolved within |
@littledan It looks like it'll be resolved in DecimalFormat. |
We need a brave soul to work on this before we go to the committee for this. |
Seems like we should add an upper limit on the right things related to numbers, to match web reality. Does anyone want to make a PR? |
FYI, ICU4C 61 now uses google/double-conversion to convert doubles into digits. It may resolve issues such as this one. |
That's great that ICU is fixed now. Does the new algorithm correspond to Number.prototype.toString or is it something else? The other side of the fix will be updating the specification text and test262 tests to verify that the right number of digits is output. |
It should in principle be the same algorithm, because it's the same library as used in V8 and elsewhere. |
This specific issue won't be fixed by ICU61, because the current algorithm still requires too much precision, just as mentioned in tc39/test262#856 (comment) ("[...] to allow some inaccuracy when x >= 1e+21.") |
Good to know it's the same algorithm, though. That means we can probably just copy the text from Number.prototype.toString. Would anyone be interested in creating such a patch? |
I seems counterintuitive to me that the spec insists on showing digits outside of the range supported by an IEEE double, which is only 15-17 significant digits. The double 1E21, or 0x444B1AE4D6E2EF50 in IEEE bytes, should be rendered as 1000000000000000000000, not as 1000000000000000012906. The stuff at the end is just noise and is outside the range that IEEE Double was designed to support. |
If ICU really needs to support un-rounded doubles, that will need to be a feature added to ICU. However, I would like to first see a discussion on whether or not this tc39/test262 spec test is actually the right behavior. |
How did you get this noise? 1E21 is stored exactly in double:
|
@sffc It sounds like ICU's behavior is good, and we should look into a specification change to match it. |
I am still confused about test262 and Yaffle's interpretation of the spec. The spec says:
It says nothing about that operation taking place in double space. In fact, the phrase "exact mathematical value" to me says, the result of the operation without floating-point noise. @Yaffle, can you please clarify? Implementation-wise, ICU 61 and higher use Google double-conversion such that conversion from numbers to strings is exact, not constrained by floating-point noise. As I said before, if ICU needs to support double values, that would be a feature that would need to be added to a future version of ICU. FYI, @FrankYFTang opened tc39/test262#2027, which changes part of test262 to reflects the current (post-61) behavior of ICU. |
BUT here the 123.445 is a IEEE 754 64-bit value, it is not a string.
it is closer to decimal123.44, than to 123.45. |
The floating-point representation of 123.445 is an implementation detail; that's the point I'm trying to make. Every IEEE double has a one-to-one mapping with a short form of 15-17 significant digits. In this case, the bits |
|
The spec does not say to do the mapping, then to do the rounding, which leads to the "double rounding". |
Okay. I also noticed that Number.prototype.toPrecision has the same language:
https://www.ecma-international.org/ecma-262/9.0/index.html#sec-number.prototype.toprecision The web reality of toFixed and toPrecision (not the intl versions) is: [3.15, 30.15, 300.15, 3000.15, 30000.15, 300000.15, 3000000.15].forEach((num) => {
console.log(num.toFixed(1) + " " + num.toPrecision(21));
}); Output (Chrome and Firefox agree):
So, the web reality of the non-intl versions of these functions is to treat doubles as their long form when rounding. This is of course different from the web reality for intl, which assumes the numbers should be treated as if they were equivalent to their short form: [3.15, 30.15, 300.15, 3000.15, 30000.15, 300000.15, 3000000.15].forEach((num) => {
console.log(num.toLocaleString("en", { maximumFractionDigits: 1 }) + " "
+ num.toLocaleString("en", { maximumSignificantDigits: 21 }));
}); Output (Chrome/FF):
I personally find the web reality Intl behavior more intuitive. Was this ever discussed in TC39 at the language level? Are there backlinks to if, how, and when it was decided to make the non-intl functions use the floating-point behavior? |
Although I can see some reasons why one could make an argument to change Intl.NumberFormat to match the behavior of Number.prototype, here are some reasons to keep the status quo.
I could almost see an argument that when fraction digits are set, short form rounding is used, and when significant digits are set, long form rounding is used. This would retain benefits 2-4. However, we would lose benefits 1 and 5. I will bring this up at next week's Ecma 402 meeting. |
The resolution from the Ecma 402 meeting is that the web reality behavior is acceptable and we should change the spec to allow for this behavior. It was suggested that the spec can specify that the direction of half rounding can be implementation dependent. |
I'd like to see any required spec work completed in time for ES 2021. Also relevant: a similar topic came up when discussing https://github.com/tc39/proposal-decimal on at TC39 on 2020-02-04. CC @waldemarhorwat. |
So, it seems, it works like at first |
We spent a great bit of time working in this problem space in the context of the Intl NumberFormat V3 proposal, which reached Stage 4 earlier this year. You can see the results of some of this effort here: https://tc39.es/ecma402/#sec-tointlmathematicalvalue I believe that |
@Yaffle found a pretty interesting case for what the spec requires and wrote a test for it at tc39/test262#856 . The test fails on multiple implementations, and the web reality may be the more intuitive answer. See that thread for details.
The text was updated successfully, but these errors were encountered: