Adding float to string kernel #1508
Merged
Conversation
This file contains 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
Signed-off-by: Haoyang Li <haoyangl@nvidia.com>
| // Range of numbers here is for normalizing the value. | ||
| // If the value is above or below the following limits, the output is converted to | ||
| // scientific notation in order to show (at most) the number of significant digits. | ||
| static constexpr double upper_limit = 10000000; // max is 1x10^7 |
Co-authored-by: Mike Wilson <hyperbolic2346@users.noreply.github.com>
Co-authored-by: Mike Wilson <hyperbolic2346@users.noreply.github.com>
Signed-off-by: Haoyang Li <haoyangl@nvidia.com>
Signed-off-by: Haoyang Li <haoyangl@nvidia.com>
Signed-off-by: Haoyang Li <haoyangl@nvidia.com>
thirtiseven
changed the title
Signed-off-by: Haoyang Li <haoyangl@nvidia.com>
…k-rapids-jni into thirtiseven-float_to_string
Signed-off-by: Haoyang Li <haoyangl@nvidia.com>
Signed-off-by: Haoyang Li <haoyangl@nvidia.com>
Signed-off-by: Haoyang Li <haoyangl@nvidia.com>
Signed-off-by: Haoyang Li <haoyangl@nvidia.com>
…k-rapids-jni into thirtiseven-float_to_string
jlowe
reviewed
Nov 14, 2023
Signed-off-by: Haoyang Li <haoyangl@nvidia.com>
Signed-off-by: Haoyang Li <haoyangl@nvidia.com>
ttnghia
reviewed
Nov 22, 2023
Signed-off-by: Haoyang Li <haoyangl@nvidia.com>
Co-authored-by: Nghia Truong <7416935+ttnghia@users.noreply.github.com>
Signed-off-by: Haoyang Li <haoyangl@nvidia.com>
ttnghia
reviewed
Nov 30, 2023
ttnghia
reviewed
Dec 1, 2023
ttnghia
reviewed
Dec 1, 2023
ttnghia
reviewed
Dec 1, 2023
ttnghia
reviewed
Dec 1, 2023
ttnghia
reviewed
Dec 1, 2023
ttnghia
reviewed
Dec 1, 2023
Signed-off-by: Haoyang Li <haoyangl@nvidia.com>
Signed-off-by: Haoyang Li <haoyangl@nvidia.com>
Signed-off-by: Haoyang Li <haoyangl@nvidia.com>
ttnghia
previously approved these changes
Dec 4, 2023
hyperbolic2346
requested changes
Dec 6, 2023
| if (d_chars != nullptr) { | ||
| float_to_string(idx); | ||
| } else { | ||
| d_offsets[idx] = compute_output_size(d_floats.element<FloatType>(idx)); |
There was a problem hiding this comment.
It seems odd to pass the index into format_float and then pass the float into compute_output_size. Why not pass the index into both for some symmetry and add auto const value = d_floats.element(idx); to compute_output_size? This is a stylistic thing and has no bearing on the output.
There was a problem hiding this comment.
good idea, done.
| * limitations under the License. | ||
| */ | ||
|
|
||
| #include "cast_string.hpp" |
There was a problem hiding this comment.
I don't understand why cast_string.hpp is quoted here. There isn't a cast_string.hpp in this directory.
Signed-off-by: Haoyang Li <haoyangl@nvidia.com>
hyperbolic2346
approved these changes
Dec 7, 2023
|
build |
|
Tested with compute-sanitizer and plugin integration tests, merging it. Thanks all for review! |
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 adds a kernel for casting float to string, to make spark-rapids produce closer results when doing such casting.
Supporting this is a necessary part of
format_numberkernel, so I split it out as a 'subtask'.This PR uses Ryū: fast float-to-string conversion (PLDI'18) as the solution for casting float/double to string. The results differ from the output of Spark's in some cases: sometimes the output is shorter (which is arguably more accurate) and sometimes the output may differ in the precise digits output (e.g., see ulfjack/ryu#83).
In most cases, the result will match Spark's results, and in the cases where it does not, the values will match when we cast them back to float. Tested in plugin PR below.
The logic part is based on ryu's C and Java implementation. I'm leaning towards keeping it consistent with ryu's codebase rather than making it more Cuda style to make it easier to apply upstream changes, but I'm not sure if that's a good practice.
Related plugin PR: NVIDIA/spark-rapids#9470