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

fix precision issue in Float64^Float64. #44529

Merged
merged 10 commits into from
Mar 12, 2022

Conversation

oscardssmith
Copy link
Member

@oscardssmith oscardssmith commented Mar 8, 2022

Closes #44525
this only costs 1 fma more, so the performance impact is minimal.

@oscardssmith oscardssmith added maths Mathematical functions embarrassing-bugfix Whoops! labels Mar 8, 2022
@KristofferC KristofferC added the needs tests Unit tests are required for this change label Mar 8, 2022
@oscardssmith
Copy link
Member Author

oscardssmith commented Mar 8, 2022

I've done local testing on 2^22 random inputs to confirm correctness. For uniform x,y (x in -0:2000, y in -100:100), mean error is now .257 ULP, maximum error found is 1.36 ULP, and only 1e-5 results have error greater than 1 ULP.

For comparison, 1.7 has slightly worse average error (.262), but better maximum error (.85 ULP)

@KristofferC
Copy link
Member

How about 10.0^-3 with this?

@oscardssmith
Copy link
Member Author

Unchanged, but that is actually a bugfix. In 1.7, 10^-3 returned a different result than 10.0^-3 due to literal pow stuff.

@oscardssmith oscardssmith added the backport 1.8 Change should be backported to release-1.8 label Mar 8, 2022
@oscardssmith
Copy link
Member Author

Also, what do you want in terms of tests for this? It's much harder to exhaustively test this than single argument functions because there are way more possibilities.

@ViralBShah
Copy link
Member

Would be nice to have a slightly more descriptive issue title.

@oscardssmith oscardssmith changed the title don't screw up pow fix precision issue in Float64^Float64. Mar 8, 2022
@giordano
Copy link
Contributor

giordano commented Mar 9, 2022

10^-3 returned a different result than 10.0^-3 due to literal pow stuff.

But now it returns a different result than BigFloat(10.0) ^ -3

julia> Float64(BigFloat(10.0) ^ -3)
0.001

@oscardssmith
Copy link
Member Author

Yeah. The 2 options are to make x^-3 3x slower and always within .51 ULP, or leave as is. I'd be fine going either way.

@KristofferC
Copy link
Member

returned a different result than 10.0^-3 due to literal pow stuff.

It kind of feels "embarrassing" to not be able to compute something like 10.0^-3 accurately anymore (this goes for both the literal pow and the floating point version). Any idea what other programming languages do here? Are we the odd one out or is this common?

@oscardssmith
Copy link
Member Author

I've created https://discourse.julialang.org/t/poll-speed-vs-performance-for-float64-float64/77619 to gauge community opinion here. I'm happy to go either way.

@oscardssmith
Copy link
Member Author

I've now fixed x^-3 to be more accurate after some democracy, and more careful analysis (I hadn't realized that the fast version's error could get above 2 ULPs). @KristofferC or anyone else that wants to this is ready to review.

@KristofferC
Copy link
Member

Still need to add tests for all the cases this changed I think?

@oscardssmith oscardssmith removed the needs tests Unit tests are required for this change label Mar 10, 2022
@oscardssmith
Copy link
Member Author

oscardssmith commented Mar 10, 2022

tests added. (note that I've run the tests locally with higher parameters, but I've reduced the number a bit to keep CI running faster).

test/math.jl Outdated Show resolved Hide resolved
test/math.jl Outdated Show resolved Hide resolved
@oscardssmith
Copy link
Member Author

The tests also caught a minor precision bug for Float32^-Integer so that is now fixed.

@KristofferC KristofferC mentioned this pull request Mar 11, 2022
47 tasks
@KristofferC
Copy link
Member

Seems to still fail CI.

@giordano
Copy link
Contributor

You may have to rebase on master

@KristofferC
Copy link
Member

Seems like some other test started failing now.

@oscardssmith
Copy link
Member Author

this is surprisingly fiddly to get 100% correct.

@oscardssmith
Copy link
Member Author

This appears to finally be correct. Thanks for catching it before release @KristofferC.

@KristofferC KristofferC merged commit 258ddc0 into JuliaLang:master Mar 12, 2022
@oscardssmith oscardssmith deleted the fix-float64-pow branch March 12, 2022 08:05
KristofferC pushed a commit that referenced this pull request Mar 12, 2022
* improve accuracy for x^-3

(cherry picked from commit 258ddc0)
@KristofferC KristofferC removed the backport 1.8 Change should be backported to release-1.8 label Mar 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
embarrassing-bugfix Whoops! maths Mathematical functions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Difference (regression?) in floating point pow on 1.8
4 participants