feat: fix and expose Float.ofScientific#14110
Merged
Merged
Conversation
Member
Author
|
!radar |
|
Benchmark results for 9a21edb against 5822584 are in. No significant results found. @TwoFX
Small changes (1✅, 1🟥)
|
|
Mathlib CI status (docs):
|
Collaborator
|
Reference manual CI status:
|
Contributor
|
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR completely rewrites the implementation of
Float.ofScientific.The new implementation passes all 5+ million tests from the
parse-number-fxx-test-datasuite. This comes at the expense of being significantly slower than the previous implementation. Unfortunately, based on my investigations, the old routine involvingFloat.scaleBseems difficult to salvage because it suffers from double-rounding issues, at least if the result turns out to be a subnormal. The algorithm described at https://www.exploringbinary.com/correct-decimal-to-floating-point-using-big-integers/ can deal with these problems, but it is also very slow.The routine implemented here is as follows: we have a fast path if both the significant and the power of ten can be represented exactly; this should happen most of the time in normal usage. Otherwise, we call out to a model implementation on
UnbundledFloatwhich manages to avoid the double-rounding issues. This is quite slow according to my measurements: the slowdown in this path is between 10x and 20x.The new implementation can be executed in the kernel, though at the moment the slow path requires either a bump to the recursion depth or the use of
decide +kernel.This means that
works now.
The full
parse-number-fxx-test-datasuite is more than 200MB in size, so we do not include it here; instead, in CI we only test 10k tests from that suite, and the full suite can be run locally if desired.Closes #13982.