Skip to content

fixed Float16 from Float64 and BigFloat#42837

Merged
oscardssmith merged 7 commits intoJuliaLang:masterfrom
oscardssmith:fix-big-to-float16
Nov 4, 2021
Merged

fixed Float16 from Float64 and BigFloat#42837
oscardssmith merged 7 commits intoJuliaLang:masterfrom
oscardssmith:fix-big-to-float16

Conversation

@oscardssmith
Copy link
Copy Markdown
Member

@oscardssmith oscardssmith commented Oct 28, 2021

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

@oscardssmith oscardssmith added bugfix This change fixes an existing bug bignums BigInt and BigFloat 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
Copy Markdown
Member Author

oscardssmith commented Oct 28, 2021

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
Copy Markdown
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
Copy Markdown
Member Author

oscardssmith commented Oct 29, 2021

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
Copy Markdown
Member

@KristofferC KristofferC Oct 30, 2021

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
Copy Markdown
Member Author

@oscardssmith oscardssmith Oct 30, 2021

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
Copy Markdown
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
Copy Markdown
Member Author

@oscardssmith oscardssmith Oct 30, 2021

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
Copy Markdown
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
@vtjnash vtjnash added the merge me PR is reviewed. Merge when all tests are passing label Nov 1, 2021
@vtjnash
Copy link
Copy Markdown
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
Copy Markdown
Member Author

oscardssmith commented Nov 1, 2021

yeah. Fixed.

@oscardssmith
Copy link
Copy Markdown
Member Author

oscardssmith commented Nov 1, 2021

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

vchuravy
vchuravy reviewed Nov 1, 2021
View reviewed changes
@oscardssmith
Copy link
Copy Markdown
Member Author

oscardssmith commented Nov 1, 2021

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

@KristofferC
Copy link
Copy Markdown
Member

KristofferC commented Nov 2, 2021

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

@oscardssmith
Copy link
Copy Markdown
Member Author

oscardssmith commented Nov 2, 2021

the c version is float64. the Julia version is bigfloat

vtjnash
vtjnash approved these changes Nov 2, 2021
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
Member Author

oscardssmith commented Nov 3, 2021

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 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
Copy Markdown
Member

KristofferC commented Nov 15, 2021

Needs a manual backport towards 1.7.

@oscardssmith
Copy link
Copy Markdown
Member Author

oscardssmith commented Nov 15, 2021

What branch should I target the PR to?

@KristofferC
Copy link
Copy Markdown
Member

KristofferC commented Nov 15, 2021

backports-release-1.7.

@oscardssmith
Copy link
Copy Markdown
Member Author

oscardssmith commented Nov 15, 2021

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
Copy Markdown
Member

KristofferC commented Mar 15, 2022

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
Closed
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 bignums BigInt and BigFloat bugfix This change fixes an existing bug float16

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@oscardssmith @gbaraldi @vtjnash @KristofferC @vchuravy @DilumAluthge