search

paritytech / parity-common

Collection of crates used in Parity projects
https://www.paritytech.io/
Apache License 2.0
290 stars 218 forks source link

Feature: Make U256::to_f64_lossy more accurate #726

Closed junderw closed 1 year ago

junderw commented 1 year ago

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 commented 1 year ago

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…

bh2smith commented 1 year ago

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 commented 1 year ago

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

ordian commented 1 year ago

@junderw could you please merge recent master?

junderw commented 1 year ago

Rebased on master.