Skip to content

[RFC] Changing the promotion type of Integer and Fixed to a float type. #192

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

Closed
kimikage opened this issue Jul 8, 2020 · 7 comments · Fixed by #207
Closed

[RFC] Changing the promotion type of Integer and Fixed to a float type. #192

kimikage opened this issue Jul 8, 2020 · 7 comments · Fixed by #207
Labels
breaking Major breaking change

Comments

@kimikage
Copy link
Collaborator

kimikage commented Jul 8, 2020

I was considering introducing checked_mul for Issue #78. However, I noticed that there are differences in promotion rules between Fixed and Normed.

julia> 2 * 0.5N2f14 # promotion to Float32
1.000061f0

julia> 2 * 0.5Q1f14 # promotion to Q1f14 (issue #78)
ERROR: ArgumentError: Fixed{Int16,14} is a 16-bit type representing 65536 values from -2.0 to 1.99994; cannot represent 2

Perhaps this is due to the current (historical) implementation where Fixed is signed and Normed is unsigned, but IMO, it seems practical to promote them to float types, and not to add ::Integer * ::FixedPoint.

@kimikage kimikage added the breaking Major breaking change label Jul 8, 2020
@kimikage
Copy link
Collaborator Author

The rules for AbstractFloat are also subtly different. I think the Normed's rule is better.

promote_rule(::Type{Fixed{T,f}}, ::Type{TF}) where {T,f,TF <: AbstractFloat} = TF

promote_rule(::Type{T}, ::Type{Tf}) where {T <: Normed,Tf <: AbstractFloat} = promote_type(floattype(T), Tf)

@kimikage
Copy link
Collaborator Author

If we decide to commonize the promotion rules, i.e. both Fixed and Normed are promoted to a float type for Integer, it seems to be "neutral" and practical that the promotion rule between Fixed and Normed returns a float types. (cf. #126)

@timholy
Copy link
Member

timholy commented Jul 11, 2020

Yes, we should go with the one used for Normed.

julia> x = rand(N0f32)
0.3137734384N0f32

julia> promote(x, Float16(1.0))
(0.3137734384075211, 1.0)

julia> x = rand(Q1f30)
-1.7184696067Q1f30

julia> promote(x, Float16(1.0))
(Float16(-1.719), Float16(1.0))

@kimikage
Copy link
Collaborator Author

kimikage commented Jul 12, 2020

I also want to commonize the following rules with #192 (comment).

@generated function promote_rule(::Type{Fixed{T1,f1}}, ::Type{Fixed{T2,f2}}) where {T1,T2,f1,f2}
f = max(f1, f2) # ensure we have enough precision
T = promote_type(T1, T2)
# make sure we have enough integer bits
i1, i2 = bitwidth(T1)-f1, bitwidth(T2)-f2 # number of integer bits for each
i = bitwidth(T)-f
while i < max(i1, i2)
Tw = widen1(T)
T == Tw && break
T = Tw
i = bitwidth(T)-f
end
:(Fixed{$T,$f})
end

Also, I'm thinking of removing the @generated. It seems that the promotion result can be inferred by replacing the while loop with recursion.

Please note that this is a "general purpose" promotion rule and we can design the promotion rules for the arithmetic operations separately.

@timholy
Copy link
Member

timholy commented Jul 12, 2020

Yes, in general my view is that promote is not entirely adequate, because "promote for what?" is a relevant question. I've personally decided that promote really means "promote for cat", meaning it computes a single concrete type used as the eltype of a container that can store both objects.

From this standpoint, even promote among Nmfn types with different n should promote to floattype:

julia> promote_type(N0f8, N1f7) === N8f8
true

julia> x = rand(N0f8)
0.651N0f8

julia> float(x) == float(convert(N8f8, x))
true

julia> x = rand(N1f7)
1.661N1f7

julia> float(x) == float(convert(N8f8, x))
false

However, different Qmfn types can promote to the one with highest f (given adequate integer widening).

@kimikage
Copy link
Collaborator Author

kimikage commented Jul 12, 2020

Even in Normed, there are cases where conversion can be performed fast and losslessly, e.g. N0f8 to N0f16.
(In that sense, the comparison on float is not fair.)

Integers as promotion targets are often Ints i.e. with 32-bit or greater width, whereas FixedPoint is usually 8-bit or 16-bit. We're avoiding Float16, so I think the promotion to floattype is often overkill.

I prefer to apply the same promotion policy for the Fixed types and the Normed types.

@kimikage
Copy link
Collaborator Author

WIP: master...kimikage:promotion_rule
This requires nbitsint, which is going to be introduced in PR #189, to support mixing signed and unsigned (NOT Fixed and Normed)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Major breaking change
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants