Implement floating point conversion with ryu #365
Conversation
|
Wonderful! We will drop older GHCs soon (see #265), do not worry about correspondent CI failures. Could you please submit tests as a separate PR? |
|
Moving off of those older GHCs is great. Any thoughts on the windows build failure Some light searching found https://gitlab.haskell.org/ghc/ghc/-/issues/18274 which suggests that there is some issue with |
IIUC we could work around this by using a different |
|
An issue with a temporary file used to happen before, but became much more prominent when Windows build switched to GHC 9.0. FIled as haskell/actions#36. Not much can be done on our side, I believe, except just rerunning tests twice or thrice |
|
Let's wait for haskell/actions#39 to land, maybe it will alleviate our issues with Windows build. |
|
|
The precomputed tables of multipliers and the formatting code both use arrays. Since they aren't exposed anywhere, I'll take a look at converting to raw address lookups |
|
@Bodigrim the |
|
@Lumaere I rerun, all green now :) I'm a bit confused: f65dde3 had tests, but they disappeared after push force. Could you please submit tests as a separate PR to be merged before this one? |
|
Can do. Some of the tests are more specific to internals (checking e.g round-trip and rounding). Should I stub those to the reference implementations or bring them back to this PR? |
Is it OK? |
|
@hsyl20 Thanks, good to know. How does We can review this with |
|
@Bodigrim The issue is that we can't link/execute generated code with GHC stage1, hence no TH splice.
Indeed. Or perhaps write a little utility program to generate the tables? Also I think we could improve the generated tables by avoiding creating double_pow5_inv_split :: Addr#
double_pow5_inv_split = "\0\1\123\45..."#We have to be careful about endianness though. Perhaps always generate little-endian tables and perform a byte-swap at runtime if the host is big-endian. |
@Lumaere sorry, missed your question. Do you mean that some tests from your branch fail under existing, I'm keen to get this merged into the next release (which is September). |
|
@Bodigrim The tests were largely ported from the original C implementation of Ryu by Ulf Adams. I made the modifications to them so that they agree with or modify all the tests to only use |
|
From my perspective, it is better to modify tests to use only |
|
Works for me. The PR is #402. Let me know if you wanted it in a different format. Thanks |
|
@la-wu could you please rebase? |
| -- doubles: 1 (sign) + 17 (mantissa) + 1 (.) + 1 (e) + 4 (exponent) = 24 | ||
| -- | ||
| maxEncodedLength :: Int | ||
| maxEncodedLength = 32 |
There was a problem hiding this comment.
So why it it 32 instead of 24?
There was a problem hiding this comment.
IIRC, I liked rounding to the nearest power of 2 for whatever reason
| boundString s = boundedPrim maxEncodedLength $ const (pokeAll s) | ||
|
|
||
| -- Sign -> Exp -> Mantissa | ||
| special :: Bool -> Bool -> Bool -> BoundedPrim () |
There was a problem hiding this comment.
Could we possibly define data Foo = Foo { fooSign :: !Bool, fooExpr :: !Bool, fooMantissa :: !Bool }, so that the meaning of arguments is self-explanatory, and their ordering is less error-prone?
There was a problem hiding this comment.
There was a problem hiding this comment.
First off, apologies for the late review! I hadn't realized that this PR had been waiting for a review for so long.
I've made a first pass over the code. So far I'm pretty impressed by the clean and well-documented internals! 👍
I should point out that I'm not very experienced with floating-point encodings, so I probably won't be able to comment on much of the internals. For correctness, I'll have to rely on the test suite, which I'm grateful you have improved so much! :)
I have a few requests:
- Please add module header haddocks to all the added modules.
- Since I have encountered several bugs that involved
fromIntegral, I have a somewhat irrational fear of it. (I also dislike that I often can't tell the types involved just from looking at it.) Would it be possible to replace its use with specializedintToFloat-style helpers? Alternatively, we could consider using-XTypeApplications, but I think that would require dropping support for GHC < 8 a bit earlier than planned so far. - I would also prefer if the types
(.>>)and(.<<)were constrained a bit, if that can be done ergonomically.
|
Thanks for the review. If you're interested in reading about the floating point string conversion, I think part 2 of the ryu paper has a good intro to it.
Will do. What do you suggest I put in the fields Copyright, Maintainer, Stability? On stability (and wrt some of your other comments that I'll get to individually as I start making edits), perhaps separating the current
I think these can both be at least partially achieved. Most (all?) of the |
Oh, I was mostly looking for brief module descriptions. I'm not sure we need all that other data. I usually skip it in other packages that I maintain. (CC @Bodigrim) Do feel free to add your copyright in the modules that you've written of course!
I do think that I'm not overly concerned about breaking changes though. As long as there's a general sense of "improvement", I think we can do a major version bump every 2 or 3 years. |
- More intuitive name for scientific notation since we're moving away from GHC.Float.FFFormat anyway
- while precision handling is not fully implemented
Data/ByteString/Builder/RealFloat.hs
Outdated
| -- "12.345" | ||
| {-# INLINABLE formatFloat #-} | ||
| formatFloat :: FloatFormat -> Float -> Builder | ||
| formatFloat (MkFloatFormat fmt prec) f = |
There was a problem hiding this comment.
I think we might want this to inline when applied to only a single argument, roughly like this:
| formatFloat (MkFloatFormat fmt prec) f = | |
| formatFloat (MkFloatFormat fmt prec) = \f -> |
@Bodigrim, what do you think?
There was a problem hiding this comment.
Yes, it will be better for inlining this way.
There was a problem hiding this comment.
I updated this. Might there be problems with assuming https://github.com/haskell/bytestring/pull/365/files#diff-3016af887ac9c7d42c56d7358eacf8168698724938c79cf8239cdd4957eaccdaR114 is lazy?
There was a problem hiding this comment.
Can you clarify what kind of problems you're concerned about? The easiest thing might be to take a look at the Core…
There was a problem hiding this comment.
Just that running f2Intermediate is unnecessary work in case of FScientific. I took a look at the dump-simpl output but it wasn't obvious to me what I should be looking for...
- `fixed` was confusing terminology adopted from FFFormat - `FormatFloat'` -> `FormatMode` - some doc tweaks
Data/ByteString/Builder/RealFloat.hs
Outdated
| -- NB: this implementation matches `show`'s output (specifically with respect | ||
| -- to default rounding and length). In particular, there are boundary cases | ||
| -- where the closest and 'shortest' string representations are not used. | ||
| -- Mentions of 'shortest' in the docs below are with this caveat. |
There was a problem hiding this comment.
This paragraph is a bit hard to understand I think. An example would be very helpful. Please also clarify what "this implementation" refers to. Maybe create a named chunk and refer to it from the relevant places.
Also note that single quotes (') are used for marking identifiers in Haddock. Escaping the quotes might work?! E.g. \'shortest\'.
There was a problem hiding this comment.
I added an example / handwavy explanation about the rounding / boundaries and pointed to the paper for details. TBH at this point I am far enough from the writing this code that I would need to redigest the exact semantics.
WRT quotes, thanks for pointing that out. I'm used to normal MD.
Data/ByteString/Builder/RealFloat.hs
Outdated
| standard :: Int -> FloatFormat | ||
| standard n = MkFloatFormat FStandard (Just n) | ||
|
|
||
| -- | Standard notation with the default precision (number of decimal places) |
There was a problem hiding this comment.
"default precision" seems like another term that could be explained in a named chunk that can be referenced from here.
There was a problem hiding this comment.
It just means matching show / formatRealFloat. LMK if I should elaborate further...
Data/ByteString/Builder/RealFloat.hs
Outdated
| in mk0 ls `mappend` mkDot rs | ||
| | otherwise -> | ||
| let (ei, is') = roundTo 10 p' (replicate (-e) 0 ++ ds) | ||
| (b:bs) = digitsToBuilder (if ei > 0 then is' else 0:is') |
There was a problem hiding this comment.
This raises Pattern match(es) are non-exhaustive with GHC 9.2.
There was a problem hiding this comment.
Hm is there a way to convince GHC that the list is non-empty? Looking at the implementation of roundTo, the non-throwing return values are (0, _) and (1, 1:_). Before the call to digitsToZero, we prepend 0 to the former case's snd value so AFIACT this is 'correct'.
There was a problem hiding this comment.
We could also just mimic the case above or add some redundant matches e.g
diff --git a/Data/ByteString/Builder/RealFloat.hs b/Data/ByteString/Builder/RealFloat.hs
index cd0d39d..0dadf45 100644
--- a/Data/ByteString/Builder/RealFloat.hs
+++ b/Data/ByteString/Builder/RealFloat.hs
@@ -258,8 +258,8 @@ showStandard m e prec =
in mk0 ls `mappend` mkDot rs
| otherwise ->
let (ei, is') = roundTo 10 p' (replicate (-e) 0 ++ ds)
- (b:bs) = digitsToBuilder (if ei > 0 then is' else 0:is')
- in b `mappend` mkDot bs
+ (ls, rs) = splitAt 1 $ digitsToBuilder (if ei > 0 then is' else 0:is')
+ in mk0 ls `mappend` mkDot rs
where p' = max p 0
where
mk0 ls = case ls of [] -> char7 '0'; _ -> mconcat ls
There was a problem hiding this comment.
I'm not saying that your code is wrong, I'm saying that we need to get rid of a warning. Boot packages cannot have warnings. Throwing an error for an empty list would be fine, for instance.
There was a problem hiding this comment.
I agree that this warning needs to be silenced. Maybe this would be a good fit for Data.List.NonEmpty?!
There was a problem hiding this comment.
I added the snippet above. A lot of the code in GHC.Float makes the same assumption though so do they just turn the warning off?
There was a problem hiding this comment.
Yes. That module uses the pragma
{-# OPTIONS_GHC -Wno-incomplete-uni-patterns #-}
|
@sjakobi sorry, I lost track of the discussion. Is there anything left unresolved? |
| -- "9.999999999999999e22" | ||
| -- | ||
| -- Simplifying, we can build a shorter, lossless representation by just using | ||
| -- @"1.0e23"@ since the floating point values that are 1 ULP away are |
There was a problem hiding this comment.
Good to know! Could you link to the article?
Note BTW that haddock supports Markdown-style hyperlinks, e.g. [ULP](https://en.wikipedia.org/wiki/Unit_in_the_last_place).
| -- NB: The float-to-string conversions exposed by this module match `show`'s | ||
| -- output (specifically with respect to default rounding and length). In | ||
| -- particular, there are boundary cases where the closest and \'shortest\' | ||
| -- string representations are not used. Mentions of \'shortest\' in the docs | ||
| -- below are with this caveat. | ||
| -- | ||
| -- For example, for fidelity, we match `show` on the output below. | ||
| -- | ||
| -- >>> show (1.0e23 :: Float) | ||
| -- "1.0e23" | ||
| -- >>> show (1.0e23 :: Double) | ||
| -- "9.999999999999999e22" | ||
| -- >>> floatDec 1.0e23 | ||
| -- "1.0e23" | ||
| -- >>> doubleDec 1.0e23 | ||
| -- "9.999999999999999e22" | ||
| -- | ||
| -- Simplifying, we can build a shorter, lossless representation by just using | ||
| -- @"1.0e23"@ since the floating point values that are 1 ULP away are | ||
| -- | ||
| -- >>> showHex (castDoubleToWord64 1.0e23) [] | ||
| -- "44b52d02c7e14af6" | ||
| -- >>> castWord64ToDouble 0x44b52d02c7e14af5 | ||
| -- 9.999999999999997e22 | ||
| -- >>> castWord64ToDouble 0x44b52d02c7e14af6 | ||
| -- 9.999999999999999e22 | ||
| -- >>> castWord64ToDouble 0x44b52d02c7e14af7 | ||
| -- 1.0000000000000001e23 | ||
| -- | ||
| -- In particular, we could use the exact boundary if it is the shortest | ||
| -- representation and the original floating number is even. To experiment with | ||
| -- the shorter rounding, refer to | ||
| -- `Data.ByteString.Builder.RealFloat.Internal.acceptBounds`. This will give us | ||
| -- | ||
| -- >>> floatDec 1.0e23 | ||
| -- "1.0e23" | ||
| -- >>> doubleDec 1.0e23 | ||
| -- "1.0e23" | ||
| -- | ||
| -- For more details, please refer to the | ||
| -- <https://dl.acm.org/doi/10.1145/3192366.3192369 Ryu paper>. |
There was a problem hiding this comment.
I think this section is a bit too technical for most users. If we want to keep it, we should probably move it out of the module header and to the bottom of the page.
There was a problem hiding this comment.
Is there such a thing as a footer? This was added per one of our discussions above about clarifying 'shortest'.
There was a problem hiding this comment.
You can insert chunks of documentation at selected places in the export list. See e.g. Data.ByteString.Lazy.
There was a problem hiding this comment.
Also see the relevant haddock documentation at https://haskell-haddock.readthedocs.io/en/latest/markup.html#controlling-the-documentation-structure.
Data/ByteString/Builder/RealFloat.hs
Outdated
| in mk0 ls `mappend` mkDot rs | ||
| | otherwise -> | ||
| let (ei, is') = roundTo 10 p' (replicate (-e) 0 ++ ds) | ||
| (b:bs) = digitsToBuilder (if ei > 0 then is' else 0:is') |
There was a problem hiding this comment.
I agree that this warning needs to be silenced. Maybe this would be a good fit for Data.List.NonEmpty?!
- add redundant pattern matching. now symmetrical with the e >= 0 case
|
@sjakobi thanks for extensive reviews! @la-wu if you get inspiration to fix documentation suggestions in a separate PR, it would be awesome. Otherwise thank you very much for all your heroic efforts to make this state-of-the-art algorithm available to Haskell users. This is a long-awaited improvement. |
* Implement floating point conversion with ryu * Use manual strictness - Strict extension is from ghc >= 8.0.1 * Use checked shifts - ghc 9.0.1 doesn't shiftL on negative unsafeShiftR and this is more straightforward regardless * Use builtin float-to-word conversion functions * Use builtin conversion to Bool * Remove dependency on array package * Handle non-exhaustive patterns * Try using prim conversions directly * Revert "Try using prim conversions directly" This reverts commit 10809d3. * Dispatch to slow cast when builtin unavailable - GHC.Float exports them starting from base-4.10.0.0 which starts with with ghc 8.2.1 * Try bumping min version to 8.4.x * Fix log10pow5 approximation and add unit test - expose RealFloat.Internal so that tests can import and verify that the approximations match in the expected ranges * Re-export floatDec and doubleDec to maintain public API * Improve documentation and fixes for initial code review - make double-polymorphic functions singly-polymorphic for clarity - use canonical Control.Arrow.first - name boolean values in special string formatting and add explanation - explain magic numbers in logarithm approximations and reciprocal multiplication - other misc comments * Improve table generation documentation and clean-up - add general overview of floating point conversion algorithm and idea behind ryu algorithm - remove unused Num Word128 instance * Improve documentation of f2s and d2s and cleanup - deduplicate shortest and rounding handling - add some comments and explanations of algorithm steps inline * Use stricter integral types and annotate fromIntegral usages - a closer inspection of `fromIntegral` usages shows that a lot of that conversion scaffolding is unnecessary if types are chose correctly - also fixes a delayed to-signed-int conversion that caused unsigned wraparound on subtraction * Add module descriptions and fix typos * Use internal FloatFormat instead of GHC.Float.FFFormat - avoids dependency especially while we don't actually support the full API of GHC.Float.formatRealFloat * Use monomorphic helpers for remaining integral conversions used by RealFloat * Remove usage of TemplateHaskell in RealFloat * Fix LUT usage on big-endian systems - Do a runtime byteswap. bswap64 is ~1 cycle on most architectures - NB: multiline string literals are parsed differently with language CPP * Add header for endianness detection * Fix big-endian word16 packing in fast digit formatting * Fix big-endian word128 read from raw addr * Clean up unused functions - finv and fnorm kept as comments for reference to table computation * Fix incorrect reciprocal function usage - Doesn't actually affect correctness vs show since round-even is not implemented (acceptBounds is always False) - Adds a couple explorative test cases and a comment anyways * Add more test coverage and fix doc example - Fixed-format fixed-precision tests - Re-exports TableGenerator constants to allow sanity checks for computed bit-range constants * Use quickcheck equality property in tests * Format haddock headers more similarly to existing ones * Use simpler reciprocal math for 32-bit words - Clarity and marginal performance improvement * Use boxed arithmetic in logic flow - more portable wrt internal ghc prim and 32- vs 64-bit - more readable (less syntax cruft in hashes and verbose operation names) - not much of a performance difference measured * Support ghc 9.2 prim word changes - Data.Word wraps fixed-sized word types - array operations now use fixed-sized word types * Fix 32-bit support - Removes most of the raw Word# usage to facilitate support of fixed-size sub-word types and 32-bit systems. Benchmarking shows little difference (<5%) - Implements manual multiplication of 64-bit words for 32-bit systems * Skip conversion to Double before fixed Float formatting - otherwise produces unnecessarily long results since imprecise representations get much more significance with Double precision * Tweak doc wording and add examples - per sjakobi suggestions * Rename FExponent to FScientific - More intuitive name for scientific notation since we're moving away from GHC.Float.FFFormat anyway * Use an opaque FloatFormat type for compatibility - while precision handling is not fully implemented * Rename float fixed-format to standard-format and other naming tweaks - `fixed` was confusing terminology adopted from FFFormat - `FormatFloat'` -> `FormatMode` - some doc tweaks * Encourage inlining by removing partial application * Fix some haddock links and accidental monospacing * Add explanation about difference between implementation and reference paper * Clarify default precision * Point to ryu paper for more details * Fix non-exhaustive warning for ghc 9.2 - add redundant pattern matching. now symmetrical with the e >= 0 case
|
@la-wu Thanks for enduring this marathon of a review! :) You may want to add your implementation to the list at https://github.com/ulfjack/ryu#ryu, BTW! |
* Implement floating point conversion with ryu * Use manual strictness - Strict extension is from ghc >= 8.0.1 * Use checked shifts - ghc 9.0.1 doesn't shiftL on negative unsafeShiftR and this is more straightforward regardless * Use builtin float-to-word conversion functions * Use builtin conversion to Bool * Remove dependency on array package * Handle non-exhaustive patterns * Try using prim conversions directly * Revert "Try using prim conversions directly" This reverts commit 10809d3. * Dispatch to slow cast when builtin unavailable - GHC.Float exports them starting from base-4.10.0.0 which starts with with ghc 8.2.1 * Try bumping min version to 8.4.x * Fix log10pow5 approximation and add unit test - expose RealFloat.Internal so that tests can import and verify that the approximations match in the expected ranges * Re-export floatDec and doubleDec to maintain public API * Improve documentation and fixes for initial code review - make double-polymorphic functions singly-polymorphic for clarity - use canonical Control.Arrow.first - name boolean values in special string formatting and add explanation - explain magic numbers in logarithm approximations and reciprocal multiplication - other misc comments * Improve table generation documentation and clean-up - add general overview of floating point conversion algorithm and idea behind ryu algorithm - remove unused Num Word128 instance * Improve documentation of f2s and d2s and cleanup - deduplicate shortest and rounding handling - add some comments and explanations of algorithm steps inline * Use stricter integral types and annotate fromIntegral usages - a closer inspection of `fromIntegral` usages shows that a lot of that conversion scaffolding is unnecessary if types are chose correctly - also fixes a delayed to-signed-int conversion that caused unsigned wraparound on subtraction * Add module descriptions and fix typos * Use internal FloatFormat instead of GHC.Float.FFFormat - avoids dependency especially while we don't actually support the full API of GHC.Float.formatRealFloat * Use monomorphic helpers for remaining integral conversions used by RealFloat * Remove usage of TemplateHaskell in RealFloat * Fix LUT usage on big-endian systems - Do a runtime byteswap. bswap64 is ~1 cycle on most architectures - NB: multiline string literals are parsed differently with language CPP * Add header for endianness detection * Fix big-endian word16 packing in fast digit formatting * Fix big-endian word128 read from raw addr * Clean up unused functions - finv and fnorm kept as comments for reference to table computation * Fix incorrect reciprocal function usage - Doesn't actually affect correctness vs show since round-even is not implemented (acceptBounds is always False) - Adds a couple explorative test cases and a comment anyways * Add more test coverage and fix doc example - Fixed-format fixed-precision tests - Re-exports TableGenerator constants to allow sanity checks for computed bit-range constants * Use quickcheck equality property in tests * Format haddock headers more similarly to existing ones * Use simpler reciprocal math for 32-bit words - Clarity and marginal performance improvement * Use boxed arithmetic in logic flow - more portable wrt internal ghc prim and 32- vs 64-bit - more readable (less syntax cruft in hashes and verbose operation names) - not much of a performance difference measured * Support ghc 9.2 prim word changes - Data.Word wraps fixed-sized word types - array operations now use fixed-sized word types * Fix 32-bit support - Removes most of the raw Word# usage to facilitate support of fixed-size sub-word types and 32-bit systems. Benchmarking shows little difference (<5%) - Implements manual multiplication of 64-bit words for 32-bit systems * Skip conversion to Double before fixed Float formatting - otherwise produces unnecessarily long results since imprecise representations get much more significance with Double precision * Tweak doc wording and add examples - per sjakobi suggestions * Rename FExponent to FScientific - More intuitive name for scientific notation since we're moving away from GHC.Float.FFFormat anyway * Use an opaque FloatFormat type for compatibility - while precision handling is not fully implemented * Rename float fixed-format to standard-format and other naming tweaks - `fixed` was confusing terminology adopted from FFFormat - `FormatFloat'` -> `FormatMode` - some doc tweaks * Encourage inlining by removing partial application * Fix some haddock links and accidental monospacing * Add explanation about difference between implementation and reference paper * Clarify default precision * Point to ryu paper for more details * Fix non-exhaustive warning for ghc 9.2 - add redundant pattern matching. now symmetrical with the e >= 0 case
|
@la-wu Out of curiosity, did you make benchmarks against the |
Hey @hsyl20, not sure if this is still relevant but I just ran a quick bench for this and I got And with double-conversion, With a setup of Not sure if I was using the library correctly... but directly converting with double-conversion to Bytestring (instead of builder) was much slower fwiw. |
|
@la-wu Thanks a lot for this report! We should definitely use your code instead of having to deal with |
This PR implements floatDec and doubleDec using the Ryu algorithm described in Ulf Adams' paper. Compared to #222, this is a native Haskell implementation based on https://github.com/Lumaere/ryu and the performance difference is relatively minimal.
However, this does add dependencies on template-haskell (which can probably be removed but will result in the hardcoding of large tables) and array.
Relevant benchmarks for this implementation on my i7-8700k
And current