-
Notifications
You must be signed in to change notification settings - Fork 8
Use significant digits only on the way in and out #66
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.