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.

Sign up for GitHub

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

Jump to bottom

fixed Float16 from Float64 and BigFloat #42837

Merged
merged 7 commits into from
Nov 4, 2021
Merged

fixed Float16 from Float64 and BigFloat #42837

merged 7 commits into from
Nov 4, 2021

Conversation

oscardssmith
Copy link
Member

previously Float16(::Float64) and Float16(::BigFloat) had double rounding issues.

@oscardssmith oscardssmith added kind:bugfix This change fixes an existing bug domain:bignums BigInt and BigFloat domain:float16 labels Oct 28, 2021
@KristofferC KristofferC added the needs tests Unit tests are required for this change label Oct 28, 2021
@oscardssmith
Copy link
Member Author

Testing these conversions are really hard because the way you generally test math is computing in higher precision and checking if the result is the same. I should probably add tests for a few likely problematic values like Float16(1.0+eps(Float16)/2 + eps(Float64)). Any other tests worth doing for this?

@vchuravy vchuravy added backport 1.6 Change should be backported to release-1.6 backport 1.7 labels Oct 28, 2021
@KristofferC KristofferC mentioned this pull request Oct 29, 2021
Merged
95 tasks
@gbaraldi
Copy link
Member

gbaraldi commented Oct 29, 2021 •
edited
Loading

Maybe one test for previous float, not sure if it would add coverage or just add clutter.

@oscardssmith
Copy link
Member Author

Tests added. This still has a bug for some values less than nextfloat(Float16(0)), but it is a major improvement.

KristofferC
KristofferC reviewed Oct 30, 2021
View reviewed changes
base/mpfr.jl Outdated
function Float16(x::BigFloat) :: Float16
res = Float32(x)
resi = reinterpret(UInt32, res)
if (resi&0x7fffffff) < 0x38800000
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Maybe this starts to need some comments now. It's a bit of magic number soup now.

Copy link
Member Author

Choose a reason for hiding this comment

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

good point. that's just if Float16(res) is subnormal.

Copy link
Sponsor Member

@KristofferC KristofferC Oct 30, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

And this is more efficient than issubnormal(Float16(res))? Does it necessarily have to be?

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah. this is only 2 instructions while converting to float16 is relatively expensive.

@oscardssmith oscardssmith removed the needs tests Unit tests are required for this change label Oct 30, 2021
@KristofferC KristofferC mentioned this pull request Oct 31, 2021
Merged
66 tasks
@oscardssmith
Copy link
Member Author

oscardssmith commented Nov 1, 2021 •
edited
Loading

Should we merge this as is? This is already a major improvement over current status and I'm not 100% sure how to fix the last remaining case the edge between nextfloat(Float16(0)) and 0

vtjnash
vtjnash reviewed Nov 1, 2021
View reviewed changes
base/mpfr.jl Show resolved Hide resolved
@vtjnash vtjnash added the status:merge me PR is reviewed. Merge when all tests are passing label Nov 1, 2021
@vtjnash
Copy link
Sponsor Member

vtjnash commented Nov 1, 2021

Do you mean all of 0.0:floatmin(Float16) or just 0.0:nextfloat(Float16(0))

@oscardssmith
Copy link
Member Author

yeah. Fixed.

@oscardssmith
Copy link
Member Author

The only test error remaining appears to be openblas threading issue.

vchuravy
vchuravy reviewed Nov 1, 2021
View reviewed changes
src/runtime_intrinsics.c Outdated Show resolved Hide resolved
@oscardssmith
Copy link
Member Author

last remaining bug fixed! from my perspective, this is ready to merge.

@KristofferC
Copy link
Sponsor Member

How is it ensured that these tests both test the C version and the Julia version?

@oscardssmith
Copy link
Member Author

the c version is float64. the Julia version is bigfloat

vtjnash
vtjnash approved these changes Nov 2, 2021
View reviewed changes
Copy link
Sponsor Member

@vtjnash vtjnash left a comment

Choose a reason for hiding this comment

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

I helped co-author this, so I will approve here too.

@oscardssmith
Copy link
Member Author

Any objections to merging this?

@oscardssmith oscardssmith merged commit 707c57c into JuliaLang:master Nov 4, 2021
@oscardssmith oscardssmith deleted the fix-big-to-float16 branch November 4, 2021 14:22
@DilumAluthge DilumAluthge removed the status:merge me PR is reviewed. Merge when all tests are passing label Nov 7, 2021
@KristofferC KristofferC mentioned this pull request Nov 15, 2021
Merged
29 tasks
@KristofferC
Copy link
Sponsor Member

Needs a manual backport towards 1.7.

@oscardssmith
Copy link
Member Author

What branch should I target the PR to?

@KristofferC
Copy link
Sponsor Member

backports-release-1.7.

@oscardssmith
Copy link
Member Author

backported: #43092 is the backport

This was referenced Nov 15, 2021
Merged
Merged
@KristofferC KristofferC mentioned this pull request Jan 10, 2022
Merged
50 tasks
LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Feb 22, 2022
@oscardssmith @LilithHafner
fixed Float16 from Float64 and BigFloat (JuliaLang#42837)
4fb4de4 
* fixed Float16 from Float64 and BigFloat. Many thanks to Jamison.
LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Mar 8, 2022
@oscardssmith @LilithHafner
fixed Float16 from Float64 and BigFloat (JuliaLang#42837)
80609dc 
* fixed Float16 from Float64 and BigFloat. Many thanks to Jamison.
@KristofferC
Copy link
Sponsor Member

Also needs a manual backport towards backports-release-1.6.

@KristofferC KristofferC mentioned this pull request May 16, 2022
Merged
45 tasks
@KristofferC KristofferC mentioned this pull request Jul 20, 2022
Open
78 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vchuravy vchuravy vchuravy left review comments

@KristofferC KristofferC KristofferC left review comments

@vtjnash vtjnash vtjnash approved these changes

Assignees
No one assigned
Labels
backport 1.6 Change should be backported to release-1.6 domain:bignums BigInt and BigFloat domain:float16 kind:bugfix This change fixes an existing bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@oscardssmith @gbaraldi @vtjnash @KristofferC @vchuravy @DilumAluthge