Use significant digits only on the way in and out#66
Conversation
dca3b87 to
1c8aa62
Compare
1c8aa62 to
2407c34
Compare
waldemarhorwat
left a comment
There was a problem hiding this comment.
Please also fix up all of the assertions that fractionDigits is nonnegative.
| 1. Else, | ||
| 1. Let _e_ be the smallest integer such that _e_ > the base 10 logarithm of abs(_value_). | ||
| 1. Let _integerDigits_ be 1 + _e_. | ||
| 1. Assert: _integerDigits_ + _fractionDigits_ > 0. |
There was a problem hiding this comment.
This assert can fail for zeroes. That's one of the reasons why I don't think we should compute significantDigits from fractionDigits.
FractionToSignificantDigits is not used anywhere. Just delete it.
|
@waldemarhorwat Thanks for the review! I wrote that FractionToSignificantDigits AO assuming that it's what would be used by the I updated the zero-handling logic to ensure that when we have a negative number of fraction digits we still have one significant digit. Another option would be to say that 0 always has exactly one significant digit (i.e. the right-most 0), while keeping the "input" part of it as it is currently in this PR (so that it's coherent with Intl). |
|
After talking with @jessealama, I'm starting to be convinced that 0 should always have 1 significant digit. In "0.000 meters", the only thing that matters is that the last digit is a 0. It's telling that we measured that distance with a tool that has 1mm resolution, and on that tool we got 0 as a result. However, to align with Intl, we should still accept non-1 significant digits as input, and normalize them. In Intl.NumberFormat, if you ask to format new Amount("0.00").fractionDigits // 2
new Amount("0.00").significantDigit // 1
new Amount(".00").fractionDigits // 2
new Amount(".00").significantDigit // 1
new Amount("0e-2").fractionDigits // 2
new Amount("0e-2").significantDigit // 1
new Amount("0e2").fractionDigits // -2
new Amount("0e2").significantDigit // 1
new Amount("0").fractionDigits // 0
new Amount("0").significantDigit // 1
new Amount("000").fractionDigits // 0
new Amount("000").significantDigit // 1
new Amount("0", { significantDigits: 3 }).fractionDigits // 2
new Amount("0", { significantDigits: 3 }).significantDigits // 1
new Amount("0", { fractionDigits: 2 }).fractionDigits // 2
new Amount("0", { fractionDigits: 2 }).significantDigits // 1This would be done by leaving the |
|
relates to #70 |
This does not match any definition of "significant digit" with which I am familiar. The only insignificant digits in an otherwise-unannotated decimal number are leading/placeholder zeros, which as a concept is notably independent of "fraction digit" (i.e., a zero to the right of the decimal point is always a fraction digit but may be significant or insignificant depending upon the digits to its left). See also Wikipedia section Rules to identify significant figures in a number. The only special behavior necessary with all-zeros input is identifying the most significant digit, and I think the best way to do so is defining it as the leftmost after normalizing leading/missing zeros left of the decimal point to a single zero:
|
|
There are still places throughout the code that either assume or assert that fractionDigits is nonnegative. Please go through them and fix them up. |
|
@gibson042 Those rules do not work with numbers whose value is 0. We need to pick which zero is the one that matters. In 0.00:
I'd argue that the right-most 0 digit is telling us something (it's telling us about the resolution of the measurement), while the left-most one is not telling us anything. 0.00m and 0cm should have exactly the same amount of significant digits, like 0.01m and 1cm do. Or also: what is that makes the left-most 0 in 0.00 significant, but does not make it significant in 0.01? |
|
|
Ok so, recap of where this PR is now. It implements the significant digits semantics as I described in #54 (comment). It does not implement what I suggested in #66 (comment), which however I would be happy to migrate to (both approaches are consistent, it comes to a matter of personal preference I guess). It possibly has a bug when it comes to @waldemarhorwat About the invalid assumptions around negative fractionDigits: |
I just did a search and found cases such as GetAmountOptions which throws for negative fractionDigits. |
OK, I'll accept that argument.
In what sense is that an exception? I haven't reviewed the changes that merged here, but did include above that |
But also, note that |
This is how I think we should handle significant digits. This PR should be though of as on top of #63.
TL;DR:
Note that we currently don't have a "on the way out" for it. I think it's just that the current spec text doesn't currently have the .fractionDigits/.significantDigits. Once those getters are added, .significantDigits should be implemented as follows:
This should match all examples from @waldemarhorwat's table in the significantDigits table from #54. When it comes to zeroes, it does the following:
I'm on the fence whether "0.0000" and ".0000" should be the same. I like that the 0 prefix is optional, but it'd be open to:
The 0e-3 case is a bit weird, but it matches that for every non-zero digit x 0.00x and xe-3 are equivalent.
Changing the behavior of .0000 and 0e-3 however requires storing both FractionalDigits and SignificantDigits, and they'd not be anymore two sides of the same coin.