-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
New floating-to-decimal formatting routine #24612
Conversation
r? @aturon (rust_highfive has picked a reviewer for you, use r? to override) |
r? @pnkfelix |
I've successfully finished
( It seems that the original float formatting code had 10KB of overhead, so the best case overhead is somewhat amortized. |
Can you here summarize the effect of "it changes the float output slightly" beyond "affects the casing of inf and nan")? In particular: There is some discussion in other tickets about things like:
I suspect that those sorts of changes are in some ways more breaking (or at least, more likely to catch developers unawares) than merely fixing our bugs with round-off error on the actual non-zero digits that we emit. And so it would be useful to know up front here whether this PR changes any of those behaviors, or if the changes to the float output is restricted to which (and how many) digits it chooses to emit on non-zero values. |
@pnkfelix It is a bit hard to say, precisely because the old code was not well-behaved. I can however surely say that this does not change the intention (say, it doesn't make
Edit 2015-04-22: I've changed the implementation so that it preserves the original behavior as much as possible. The changes specific to the original code (but subsequently reverted in this PR) are as follows:
Regarding the existing discussions, with an exception of the sign of |
limitation `limit` (which determines the actual `n`), and we would like to get | ||
the representation `V = 0.d[0..n-1] * 10^k` such that: | ||
|
||
- `d[0]` is non-zero, unless `n` was zero in which case only `k` is returned. |
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.
I don't understand how the returned value for k
is meaningful in the case where n
is 0 ... is this comment implying that in such a scenario, the returned d[0]
would have been non-zero if n
had been positive?
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.
No. For example, v = 3.14
and limit = 3
gives n = 0
and k = 3
. The extrapolated digits would start with 00314...
.
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.
Ah, re-reading your comment I understand what you really meant; yes, k <= limit
holds if and only if n = 0
. The exact value of k
is thus unused in that case, but the code has a debugging assertion to check this identity.
There actually was one wrong comment (derived from the older faulty implementation) on to_exact_fixed_str
implying that n
cannot be zero (oops). I'll fix that soon.
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.
No, I was and still am confused. In the comment above, n
is the number of digits that we need to extract from the array d
in the representation V = 0.d[0..n-1] * 10^k
. My comments were based on that equation; it led me to ask the question: when can n
be zero?
At the time when I wrote that question, I had not yet read the paragraph that starts with
When
limit
is given but notn
, ...
which seems to describe scenarios that yield non-positive values for n
. But this mostly just confused me further, probably because I do not yet actually understand the conversion that you have named "fixed mode conversion".
The text in that paragraph that makes it sound like the equation V = 0.d[0..n-1] * 10^k
simply cannot be well-defined ... maybe that is the source of my confusion.
I will try reading the whole text here and see if I can suggest a way to rephrase it to be clearer.
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.
(BTW I do recognize that the equation for V
makes sense for n = 0
when V = 0
itself. But you yourself gave the example of v = 3.14
; so clearly I am still not understanding something here.)
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.
Probably I've introduced the concepts backwards.
- The "exact mode conversion" is the mode that the caller demands the specified number of significant digits.
- The "fixed mode conversion" is the mode that the caller demands the specified number of fractional digits.
The fixed mode can be implemented via the exact mode and the estimator, which calculates the required number of significant digits from the number of fractional digits and the original value. It turns out that the internal digit generation code already has to estimate the exponent (which is almost same to the difference between them), so we can reuse this logic.
In order to achieve this merger, the combined exact-fixed mode is implemented so that both the number of significant digits len
and the number of fractional digits frac_digits
are given. (The actual interface differs from this to be more flexible: len == buf.len()
and frac_digits == -limit
. This doesn't change the discussion however.) The digit generation code will finish whichever comes first, so it is possible that there are no digits generated. The n
as I originally described is really determined from either len
or frac_digits
, so it might have been confusing.
By the way, I realized that my original example doesn't arise with the current external interface. So I'll give a better example: v = 0.000045
, len = 3
, frac_digits = 3
(so that limit = -3
). Since v = 0.45 * 10^-4
, k = -4
, but this clearly triggers the condition k <= limit
. This means that the resulting representation is all zeroes, i.e. V = 0.000
(note that k
is unused here).
(The better description for this is always welcomed. 😭)
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.
Oh wait! Are you actually saying:
we would like to get the representation
V = 0.d[0..n-1] * 10^k
such thatd[0]
is non-zero, unlessn
was zero in which casek
digits are returned.
That I could understand, I think!
I will try re-reading this text, but substituting e.g. "we will only get k
digits" in the spots where you wrote "we will only get k
", and see if it all becomes clear to me then.
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.
Uh, no. k
is just the exponent which does not depend on the determined n
(which is the number of digits actually returned). Say that, for certain v
, the algorithm returned 0.123 * 10^-8
(which implies that n = 3
, and also implies either len = 3
or limit = -11
). Then for some other values for limit
, it may return 0. * 10^-8
(with n = 0
and k = -8
). 10^-8
part still remains here, therefore "we will only get k
".
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.
Oh sorry, somehow I didn't hit "reload" before I posted my last comment, so it may seem like I completely ignored you in my last comment.
I am re-reading the Steele+White Dragon paper now; I think your terminology matches theirs, so I just need to digest section 8 of their paper and figure out the right way to succinctly describe it.
Though to be honest the comment you wrote that points out the distinction between requesting a number of significant digits versus a number of fractional digits has helped me a lot.
Okay, back to reading, thanks.
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.
You are welcome. Actually, I guess my comment above can directly go to the module documentation...
The JSON: let s = v.to_string();
if s.contains(".") {s} else {s + ".0"}
The CSV serializer works because CSV: let s: String = format!("{:.10}", v).trim_right_matches('0').into();
if s.ends_with('.') { s + "0" } else { s } |
@rprichard Aargh, I forgot to mention that! I have updated the earlier comment to include that breaking change. Thank you so much. (As mentioned above, this too is very easy to change though. If the decision is reached I'll rebase this commit. I explicitly mentioned this kind of decision in the "known issues" to gather the future direction, as it seems that there are ongoing discussions about changing defaults.) |
@lifthrasiir hmm, it will probably be easier to land the most important parts of this PR (namely, the bug fixes) if you remove the big breaking change(s) like the one that @rprichard mentioned. (Hopefully we could figure out a way for a user to opt-in to getting the But, I'll make sure we talk about this at the team meeting tonight. Maybe we can go ahead and change this without going through an RFC. |
@pnkfelix Yeah, that was my oversight, I forgot to replicate the old behavior in some cases. I'll rebase this tonight (i.e. within 6 hours) to avoid any breaking changes except for fixing the rounding and inaccurate result. |
FWIW, the |
self.base[i] = 0; | ||
} | ||
|
||
// shift by `nbits` bits |
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.
nit: nbits
must be referring to bits
here, right?
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.
Yes.
97c76cb
to
dccacbc
Compare
/// The integer mantissa. | ||
pub f: u64, | ||
/// The exponent in base 2. | ||
pub e: i16, |
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.
I haven't read the Grisu paper carefully, but in its discussion of the diy_fp
struct, it points out
- (1.) the paper's definition uses an unlimited range value for
e_x
to simplify the proofs, - (2.) in practice the exponent type must only have "slightly greater range than the input exponent" and
- (3.) for IEEE doubles which have 11-bit exponents, a 32-bit signed integer is by far big enough.
The combination of 2. and 3. leads me to wonder whether we could get into trouble using only i16
here, which only has five extra bits. I know you exhaustively tested your algorithm for f32
, but how confident are you that i16
suffices for f64
?
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.
That is more than enough. Grisu does have a stricter limitation on the mantissa size (Grisu1 needs 2 more extra bits, Grisu2 and Grisu3 needs 3), but they are very generous about the exponent range.
diy_fp
is converted from the decoded float, then normalized, then multiplied by the cached power of 10 to get that within a certain interval (to be exact, [4, 2^32)
). Since the normalization step can decrease the exponent by at most 63, and the original float has the minimal exponent of -1075, the exponent is no less than -1138. With the same argument, the exponent is no more than 971.
(In fact, this exponent type is enough for f80
if Rust had support for that. The problem with f80
is, however, that its mantissa is too large and u64
would not be sufficient.)
(By the way this is all looking pretty fantastic so far. I just want to really make sure I or someone else besides you has a decent understanding of all of it, so please do bear with me while I continue to work my way through it.) |
@lifthrasiir okay, thanks for your patience with this. I'm going to r+ this. I cannot claim that I understand the algorithms 100%, but I have reviewed all the papers and satisfied myself that this implementation is a good match for them, and that the goals of the papers are a match for what we want in Rust. |
epic! |
⌛ Testing commit 3d34e17 with merge d8b3a6a... |
💔 Test failed - auto-mac-64-opt |
Whoops, I forgot to run |
I've confirmed this new commit passes |
This is a direct port of my prior work on the float formatting. The detailed description is available [here](https://github.com/lifthrasiir/rust-strconv#flt2dec). In brief, * This adds a new hidden module `core::num::flt2dec` for testing from `libcoretest`. Why is it in `core::num` instead of `core::fmt`? Because I envision that the table used by `flt2dec` is directly applicable to `dec2flt` (cf. #24557) as well, which exceeds the realm of "formatting". * This contains both Dragon4 algorithm (exact, complete but slow) and Grisu3 algorithm (exact, fast but incomplete). * The code is accompanied with a large amount of self-tests and some exhaustive tests. In particular, `libcoretest` gets a new dependency on `librand`. For the external interface it relies on the existing test suite. * It is known that, in the best case, the entire formatting code has about 30 KBs of binary overhead (judged from strconv experiments). Not too bad but there might be a potential room for improvements. This is rather large code. I did my best to comment and annotate the code, but you have been warned. For the maximal availability the original code was licensed in CC0, but I've also dual-licensed it in MIT/Apache as well so there should be no licensing concern. This is [breaking-change] as it changes the float output slightly (and it also affects the casing of `inf` and `nan`). I hope this is not a big deal though :) Fixes #7030, #18038 and #24556. Also related to #6220 and #20870. ## Known Issues - [x] I've yet to finish `make check-stage1`. It does pass main test suites including `run-pass` but there might be some unknown edges on the doctests. - [ ] Figure out how this PR affects rustc. - [ ] Determine which internal routine is mapped to the formatting specifier. Depending on the decision, some internal routine can be safely removed (for instance, currently `to_shortest_str` is unused).
Awesome! Epic review @pnkfelix, and thanks to @lifthrasiir for implementing these tricky algorithms. |
Cool! Finally a proper implementation for this stuff :) |
- Makes Num stringification 2x faster (tested with rand.Str) - Fixes RT#127184 https://rt.perl.org/Ticket/Display.html?id=127184 - Fixes RT#124796 https://rt.perl.org/Ticket/Display.html?id=124796 - Fixes RT#132330 https://rt.perl.org/Ticket/Display.html?id=132330 (fixes Num.WHICH and problems with set()s mentioned in that ticket) Grisu3[^1] is a 2010 algorithm that stringifies doubles to shortest possible representation that would still result in the same value if it's parsed and stringified again. This sort of strigification is what fixes the reported bugs. As a bonus, the algo is faster than snprintf('%.15g') we used to use for stringification. Grisu3 handles ~99.5% of cases. In the remaining 0.5%, it knows it won't produce the shortest possible result and so a fallback to slower algorithm is made, which in our current case is snprintf('%.17g'), which is a suboptimal fallback, and in the future Dragon4 algo[^2] could be used instead. The change from .15g to .17g in the fallback is intentional, as 15 is the number of guaranteed significant digits, but 17 is the number of maximum available significant digits when the IEEE doubles differ by at most one unit[^7]. Based on my research, an improved fallback would be Dragon4 algo[^2], and Grisu3+Dragon4 is the current state of the art. (some references may state that Errol[^8] algo superseded Grisu3+Dragon4 combo, but based on authors' corrections[^9], it appears there was a benchmarking error and Errol is not in fact faster). The Grisu3+Dragon4 is used[^5] by Rust (see also some trial impls in [^6]) and Grisu3's author's C++ version of the code[^3] indicates it's used by V8 JavaScript engine as well. There exist a C# impl[^4] of the algo as well, used in an efficient JSON encoder. [1] "Printing Floating-Point Numbers Quickly and Accurately with Integers" by Florian Loitsch https://goo.gl/cbvogg [2] "How to Print Floating-Point Numbers Accurately" by Steel & White: https://lists.nongnu.org/archive/html/gcl-devel/2012-10/pdfkieTlklRzN.pdf [3] https://github.com/google/double-conversion [4] https://github.com/kring/grisu.net [5] rust-lang/rust#24612 [6] https://github.com/lifthrasiir/rust-strconv [7] http://www2.open-std.org/JTC1/SC22/WG21/docs/papers/2005/n1822.pdf [8] https://github.com/marcandrysco/Errol [9] https://github.com/marcandrysco/Errol#performance-evaluation-correction
- Makes Num stringification 2x faster (tested with rand.Str) - Fixes RT#127184 https://rt.perl.org/Ticket/Display.html?id=127184 - Fixes RT#132330 https://rt.perl.org/Ticket/Display.html?id=132330 (fixes Num.WHICH and problems with set()s mentioned in that ticket) Grisu3[^1] is a 2010 algorithm that stringifies doubles to shortest possible representation that would still result in the same value if it's parsed and stringified again. This sort of strigification is what fixes the reported bugs. As a bonus, the algo is faster than snprintf('%.15g') we used to use for stringification. Grisu3 handles ~99.5% of cases. In the remaining 0.5%, it knows it won't produce the shortest possible result and so a fallback to slower algorithm is made, which in our current case is snprintf('%.17g'), which is a suboptimal fallback, and in the future Dragon4 algo[^2] could be used instead. The change from .15g to .17g in the fallback is intentional, as 15 is the number of guaranteed significant digits, but 17 is the number of maximum available significant digits when the IEEE doubles differ by at most one unit[^7]. Based on my research, an improved fallback would be Dragon4 algo[^2], and Grisu3+Dragon4 is the current state of the art. (some references may state that Errol[^8] algo superseded Grisu3+Dragon4 combo, but based on authors' corrections[^9], it appears there was a benchmarking error and Errol is not in fact faster). The Grisu3+Dragon4 is used[^5] by Rust (see also some trial impls in [^6]) and Grisu3's author's C++ version of the code[^3] indicates it's used by V8 JavaScript engine as well. There exist a C# impl[^4] of the algo as well, used in an efficient JSON encoder. [1] "Printing Floating-Point Numbers Quickly and Accurately with Integers" by Florian Loitsch https://goo.gl/cbvogg [2] "How to Print Floating-Point Numbers Accurately" by Steel & White: https://lists.nongnu.org/archive/html/gcl-devel/2012-10/pdfkieTlklRzN.pdf [3] https://github.com/google/double-conversion [4] https://github.com/kring/grisu.net [5] rust-lang/rust#24612 [6] https://github.com/lifthrasiir/rust-strconv [7] http://www2.open-std.org/JTC1/SC22/WG21/docs/papers/2005/n1822.pdf [8] https://github.com/marcandrysco/Errol [9] https://github.com/marcandrysco/Errol#performance-evaluation-correction
This is a direct port of my prior work on the float formatting. The detailed description is available here. In brief,
core::num::flt2dec
for testing fromlibcoretest
. Why is it incore::num
instead ofcore::fmt
? Because I envision that the table used byflt2dec
is directly applicable todec2flt
(cf. Float printing and/or parsing is inaccurate #24557) as well, which exceeds the realm of "formatting".libcoretest
gets a new dependency onlibrand
. For the external interface it relies on the existing test suite.This is rather large code. I did my best to comment and annotate the code, but you have been warned.
For the maximal availability the original code was licensed in CC0, but I've also dual-licensed it in MIT/Apache as well so there should be no licensing concern.
This is [breaking-change] as it changes the float output slightly (and it also affects the casing of
inf
andnan
). I hope this is not a big deal though :)Fixes #7030, #18038 and #24556. Also related to #6220 and #20870.
Known Issues
make check-stage1
. It does pass main test suites includingrun-pass
but there might be some unknown edges on the doctests.to_shortest_str
is unused).