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

[RFC] Supporting checked arithmetic for division-related operations #221

Closed
kimikage opened this issue Aug 22, 2020 · 3 comments · Fixed by #230
Closed

[RFC] Supporting checked arithmetic for division-related operations #221

kimikage opened this issue Aug 22, 2020 · 3 comments · Fixed by #230

Comments

@kimikage
Copy link
Collaborator

While there is room for some improvement and discussion, I have prepared the implementations for checked arithmetic on addition and multiplication (cf. #41 (comment)).

However, the circumstances surrounding division-related operations are more complicated.

Checked arithmetic for /

Since the result of the / with integer arguments is a floating point number, the checked version of / is not defined in Base.Checked. Unlike * and mul, / and div are different operations. So, checked_div is for div(÷) and not for /.
Shall we define {checked/saturating/wrapping}_fdiv? ("f" stands for "fractional", not "float" 😄).

Default arithmetic

Unlike addition and subtraction, the operations involved in integer division are basically "checked" arithmetic. It is not clear whether we should follow the integer style or the floating point style (which does not throw an exception). However, I personally think the checked arithmetic is a better choice for the default, since FixedPoint numbers cannot represent Inf or NaN.

Division by zero in wrapping arithmetic

There are several options for the result of dividing by zero. Considering the distinction from saturating arithmetic, I think it is a good idea to return zero.

@kimikage
Copy link
Collaborator Author

kimikage commented Aug 25, 2020

BTW, as with issue #173, the / for Fixed has a rounding mode of RoundNearest.

/(x::Fixed{T,f}, y::Fixed{T,f}) where {T,f} = Fixed{T,f}(div(convert(widen(T), x.i) << f, y.i), 0)

julia> a = Q0f7(11/128) / Q0f7(96/128)
0.109Q0f7

julia> b = Q0f7(11/96)
0.117Q0f7

julia> a - 11/96, b - 11/96
(-0.005208333333333329, 0.0026041666666666713)

Unlike multiplication, the SIMD instruction for integer division does not exist in x86 (SSE, AVX), so the method of converting to floating point numbers may be effective.

/(x::T, y::T) where {T <: Normed} = convert(T,convert(floattype(T), x)/convert(floattype(T), y))

@kimikage
Copy link
Collaborator Author

kimikage commented Sep 3, 2020

Return type of div/cld/fld/fld1

Currently, div/fld/fld1 return a value in the integer rawtype instead of the input FixedPoint type. This is a reasonable choice to save the bit width.
(BTW, surprisingly, support for cld is missing. 😅)

for f in (:(==), :<, :<=, :div, :fld, :fld1)

However, in PR #207, we decided to promote Integer and FixedPoint to a floating-point type. (That was the case with Normed even before PR #207.) That is not very convenient when used in combination with rem and mod, which return a FixedPoint number.

One solution is to add widemul(x::T, y::X) where {T <: Integer, X:< FixedPoint{T}} as a measure to avoid affecting promote_rule. (cf. #185)

BTW, I plan to keep fld1 and mod1 as they are (without adding wrapping_fld1 etc.).

@kimikage
Copy link
Collaborator Author

kimikage commented Sep 9, 2020

Return value of saturating_mod and saturating_rem.

julia> x = typemin(Q0f7); y = -eps(Q0f7);

julia> saturating_div(x, y) # 128 --> 127
127

julia> x == saturating_div(x, y) * y + saturating_rem(x, y) # should be true?

If the equation above should be always true, saturating_rem(typemin(Q0f7), -eps(Q0f7)) == -eps(Q0f7).
If abs(saturating_rem(x, y)) < abs(y) should be always true, saturating_rem(typemin(Q0f7), -eps(Q0f7)) == zero(Q0f7).

I consider the former to be the better option.

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 a pull request may close this issue.

1 participant