Skip to content
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

Feature: Make U256::to_f64_lossy more accurate #726

Merged
merged 1 commit into from
Jul 12, 2023

Conversation

junderw
Copy link
Contributor

@junderw junderw commented Mar 13, 2023

Based off of this blog post: https://blog.m-ou.se/floats/ (I tried to get the gist of this into the comments, but it's kind of long.)

This method of converting large unsigned integers into f64 is more accurate, but from my rough benchmarks, for most numbers there's at least a 3x slow down, and for some numbers (that work well with the simple method currently used) it's as much as a 10x slow down for speed.

Sacrificing speed for accuracy.

Please close if this is a trade off that's not acceptable.

I'd assume anyone dealing with floats is not expecting accuracy, but at the same time, I'm not too sure exactly how the current method's inaccuracies will manifest themselves.

Anywho, I was doing this work for another PR and thought it might be useful here.

@ggwpez
Copy link
Member

ggwpez commented Apr 28, 2023

This method of converting large unsigned integers into f64 is more accurate, but from my rough benchmarks, for most numbers there's at least a 3x slow down, and for some numbers (that work well with the simple method currently used) it's as much as a 10x slow down for speed.

Thanks for mentioning this.
We should probably add benchmarks here as well to compare. Anyway, i also think we should favour correctness.

And probably a fuzzer test to check that it is correct now…

@ggwpez ggwpez requested a review from achimcc April 28, 2023 11:22
Copy link
Member

@ordian ordian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for delay.

We're not using fp-conversion feature ourselves, so I won't be blocking the PR lacking knowledge of fp and the use-cases you're after.

cc @bh2smith @fleupold who introduced this feature

@bh2smith
Copy link
Contributor

Thanks for the ping @ordian -- I see that no tests have been changed and everything is passing, so I imagine the calculation has not changed so much in the end. Seems fine by me!

@ordian
Copy link
Member

ordian commented Jul 10, 2023

Hmm, actually I don't see CI checks running, maybe a fluke with GitHub CI.

@ordian
Copy link
Member

ordian commented Jul 10, 2023

@junderw could you please merge recent master?

@junderw
Copy link
Contributor Author

junderw commented Jul 10, 2023

Rebased on master.

@ordian ordian added the changelog Needs to be added to the changelog label Jul 11, 2023
@ordian ordian merged commit dce4bbd into paritytech:master Jul 12, 2023
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog Needs to be added to the changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants