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

BFloat16(::BigFloat) and BigFloat(::BFloat16) #63

Merged
merged 2 commits into from
Sep 17, 2024
Merged

Conversation

milankl
Copy link
Member

@milankl milankl commented Dec 28, 2023

We currently have

julia> BigFloat(BFloat16(1))
ERROR: MethodError: no method matching BigFloat(::BFloat16)

and

julia> convert(BFloat16,BigFloat(1))
ERROR: MethodError: no method matching BFloat16(::BigFloat)

This PR just defines those conversions via Float32, outside of the llvm_arithmetic blocks because I believe there's no LLVM functionality for BigFloat hence his needs to happen independently anyway.

Copy link

codecov bot commented Dec 28, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 66.66%. Comparing base (4b657c0) to head (e415c29).
Report is 23 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master      #63      +/-   ##
==========================================
+ Coverage   66.29%   66.66%   +0.36%     
==========================================
  Files           3        3              
  Lines         181      183       +2     
==========================================
+ Hits          120      122       +2     
  Misses         61       61              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@milankl milankl requested a review from maleadt December 28, 2023 14:45
@maleadt
Copy link
Member

maleadt commented Jan 1, 2024

This seems sensible, especially with bfloat being a truncated float, but I'm no floating-point expert so there may be an improved conversion. cc @oscardssmith maybe?

@milankl
Copy link
Member Author

milankl commented Jan 1, 2024

Maybe there's a way to do this faster, I don't know how Float32(::BigFloat) and the inverse are implemented. But Float32 is a superset of BFloat16 so you don't lose any precision if you convert to Float32 first.

@oscardssmith
Copy link
Member

the BFloat16(::BigFloat) version of this will differ double rounding. I suggest looking at how the float16 conversation works in Julia

@milankl
Copy link
Member Author

milankl commented Jan 16, 2024

I see because of the cases where a-------x-b--------c, with a,c BFloat16s, b=(a+c)/2, a Float32 then x could be rounded to b::Float32 and subsequently to c even though it would round to a in a direct conversion

@milankl
Copy link
Member Author

milankl commented Jan 16, 2024

Wouldn't this happen here too?

for F in (:abs, :abs2, :sqrt, :cbrt,
:exp, :exp2, :exp10, :expm1,
:log, :log2, :log10, :log1p,
:sin, :cos, :tan, :csc, :sec, :cot,
:asin, :acos, :atan, :acsc, :asec, :acot,
:sinh, :cosh, :tanh, :csch, :sech, :coth,
:asinh, :acosh, :atanh, :acsch, :asech, :acoth)
@eval begin
Base.$F(x::BFloat16) = BFloat16($F(Float32(x)))
end
end

@oscardssmith
Copy link
Member

yes, but getting the elementary functions rounded correctly isn't a requirement (or easy), but correct rounding for arithmetic and conversion are relatively easy.

@ViralBShah
Copy link
Member

Merge?

@maleadt maleadt requested review from oscardssmith and removed request for maleadt September 17, 2024 12:16
@oscardssmith
Copy link
Member

as previously mentioned this version has double rounding, which can be avoided by using the same algorithm Float16 uses, but this is better than not having the capability

@maleadt
Copy link
Member

maleadt commented Sep 17, 2024

Thanks; so let's merge this and open an issue to track the improvement.

@maleadt maleadt merged commit 7cf9374 into master Sep 17, 2024
18 checks passed
@maleadt maleadt deleted the mk/bigfloat branch September 17, 2024 14:48
@maleadt maleadt mentioned this pull request Sep 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants