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

Add checked math to FixedDecimals; default to overflow behavior #85

Merged
merged 21 commits into from
Dec 19, 2023
Merged

Add checked math to FixedDecimals; default to overflow behavior #85

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
c81bb83
Let all the FixedDecimals operations overflow, matching Int overflow
NHDaly Nov 24, 2023
9da03cd
Add checked_* methods for FixedDecimals
NHDaly Dec 7, 2023
f19c147
Fix dispatch for checked_mul and checked_div. Fix checked_div
NHDaly Dec 7, 2023
ea46f72
Add checked `/` operation for FDs: `checked_decimal_division`
NHDaly Dec 7, 2023
c465e6f
Add README sections around overflow, conversions and inexact errors
NHDaly Dec 7, 2023
fc1d927
Add the missing checked_ operations: fld,cld,rem,mod,abs,neg. Add tests
NHDaly Dec 7, 2023
e134ee4
Fix up tests
NHDaly Dec 9, 2023
2efb468
Add corner cases tests
NHDaly Dec 9, 2023
7793ffc
Fix OverflowError for older versions of julia
NHDaly Dec 9, 2023
30aef4f
Add fld/cld tests, but they still seem wrong, and abs/neg
NHDaly Dec 9, 2023
07bf40f
Change overflow tests to FD{Int8,1} to make the results easier to thi…
NHDaly Dec 9, 2023
137c8b4
Add promotions tests
NHDaly Dec 9, 2023
e1dd56d
Rename checked_rdiv; reexport checked*
NHDaly Dec 18, 2023
d119dce
Enable nightly CI
NHDaly Dec 18, 2023
41a69fd
Apply suggestions from code review
NHDaly Dec 18, 2023
3270ad5
Improve perf: don't force-promote inside *()
NHDaly Dec 18, 2023
4cb2d5d
Make division operations always throw on overflow!
NHDaly Dec 19, 2023
0cf9092
Add overflow tests for fld1 and div(::RoundMode)
NHDaly Dec 19, 2023
8eefc80
Cleanup
NHDaly Dec 19, 2023
34a9306
This is a breaking release!
NHDaly Dec 19, 2023
73410da
compat to README
NHDaly Dec 19, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 57 additions & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,3 +44,60 @@ julia> 0.1 + 0.2
julia> FixedDecimal{Int,1}(0.1) + FixedDecimal{Int,1}(0.2)
FixedDecimal{Int64,1}(0.3)
```

### Arithmetic details: Overflow and checked math

By default, all arithmetic operations on FixedDecimals **will silently overflow**, following the standard behavior for bit integer types in Julia. For example:
```julia
julia> FixedDecimal{Int8,2}(1.0) + FixedDecimal{Int8,2}(1.0)
FixedDecimal{Int8,2}(-0.56)

julia> -FixedDecimal{Int8,2}(-1.28) # negative typemin wraps to typemin again
FixedDecimal{Int8,2}(-1.28)

julia> abs(FixedDecimal{Int8,2}(-1.28)) # negative typemin wraps to typemin again
FixedDecimal{Int8,2}(-1.28)
```

In most applications dealing with `FixedDecimals`, you will likely want to use the **checked arithmetic** operations instead. These operations will _throw an OverflowError_ on overflow or underflow, rather than silently wrapping. For example:
```julia
julia> Base.checked_mul(FixedDecimal{Int8,2}(1.2), FixedDecimal{Int8,2}(1.2))
ERROR: OverflowError: 1.20 * 1.20 overflowed for type FixedDecimal{Int8, 2}

julia> Base.checked_add(FixedDecimal{Int8,2}(1.2), 1)
ERROR: OverflowError: 1.20 + 1.00 overflowed for type FixedDecimal{Int8, 2}

julia> Base.checked_div(Int8(1), FixedDecimal{Int8,2}(0.5))
ERROR: OverflowError: 1.00 ÷ 0.50 overflowed for type FixedDecimal{Int8, 2}
```

**Checked division:** Note that `checked_div` performs truncating, integer division. Julia Base does not provide a function to perform checked decimal division, so we provide one in this package, `FixedPointDecimals.checked_decimal_division`.

Here are all the checked arithmetic operations supported by `FixedDecimal`s:
- `Base.checked_add(x,y)`
- `Base.checked_sub(x,y)`
- `Base.checked_mul(x,y)`
- `Base.checked_div(x,y)`
- `FixedPointDecimals.checked_decimal_division(x,y)`
- `Base.checked_cld(x,y)`
- `Base.checked_fld(x,y)`
- `Base.checked_rem(x,y)`
- `Base.checked_mod(x,y)`
- `Base.checked_neg(x)`
- `Base.checked_abs(x)`

### Conversions, Promotions, and Inexact Errors.

Note that arithmetic operations will _promote_ all arguments to the same FixedDecimal type
before performing the operation. If you are promoting a non-FixedDecimal _number_ to a FixedDecimal, there is always a chance that the Number will not fit in the FD type. In that case, the conversion will throw an exception. Here are some examples:
```julia
julia> FixedDecimal{Int8,2}(2) # 200 doesn't fit in Int8
ERROR: InexactError: convert(FixedDecimal{Int8, 2}, 2)

julia> FixedDecimal{Int8,2}(1) + 2 # Same here: 2 is promoted to FD{Int8,2}(2)
ERROR: InexactError: convert(FixedDecimal{Int8, 2}, 2)

julia> FixedDecimal{Int8,2}(1) + FixedDecimal{Int8,1}(2) # Promote to the higher-precision type again throws.
ERROR: InexactError: convert(FixedDecimal{Int8, 2}, 2.0)
```

130 changes: 123 additions & 7 deletions src/FixedPointDecimals.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,13 +187,13 @@

# these functions are needed to avoid InexactError when converting from the
# integer type
Base.:*(x::Integer, y::FD{T, f}) where {T, f} = reinterpret(FD{T, f}, T(x * y.i))
Base.:*(x::FD{T, f}, y::Integer) where {T, f} = reinterpret(FD{T, f}, T(x.i * y))
Base.:*(x::Integer, y::FD{T, f}) where {T, f} = reinterpret(FD{T, f}, *(promote(x, y.i)...))
Base.:*(x::FD{T, f}, y::Integer) where {T, f} = reinterpret(FD{T, f}, *(promote(x.i, y)...))
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think when the Integer is a BigInt, and T is not, the promote would allocate another bigint which might not be needed because there are usually specialized methods for BigInt x Integer that avoid the allocation.

Copy link
Member Author

Choose a reason for hiding this comment

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

So maybe i should just leave it without the promote() and let * do the promotion internally if needed? I'll try that


function Base.:/(x::FD{T, f}, y::FD{T, f}) where {T, f}
powt = coefficient(FD{T, f})
quotient, remainder = fldmod(widemul(x.i, powt), y.i)
reinterpret(FD{T, f}, T(_round_to_nearest(quotient, remainder, y.i)))
reinterpret(FD{T, f}, _round_to_nearest(quotient, remainder, y.i))
end

# These functions allow us to perform division with integers outside of the range of the
Expand All @@ -202,12 +202,12 @@
powt = coefficient(FD{T, f})
powtsq = widemul(powt, powt)
quotient, remainder = fldmod(widemul(x, powtsq), y.i)
reinterpret(FD{T, f}, T(_round_to_nearest(quotient, remainder, y.i)))
reinterpret(FD{T, f}, _round_to_nearest(quotient, remainder, y.i))
end

function Base.:/(x::FD{T, f}, y::Integer) where {T, f}
quotient, remainder = fldmod(x.i, y)
reinterpret(FD{T, f}, T(_round_to_nearest(quotient, remainder, y)))
reinterpret(FD{T, f}, _round_to_nearest(quotient, remainder, y))
end

# integerification
Expand Down Expand Up @@ -362,14 +362,130 @@
for divfn in [:div, :fld, :fld1, :cld]
# div(x.i, y.i) eliminates the scaling coefficient, so we call the FD constructor.
# We don't need any widening logic, since we won't be multiplying by the coefficient.
@eval Base.$divfn(x::T, y::T) where {T <: FD} = T($divfn(x.i, y.i))
#@eval Base.$divfn(x::T, y::T) where {T <: FD} = T($divfn(x.i, y.i))
# @eval Base.$divfn(x::T, y::T) where {T <: FD} = $divfn(promote(x.i, y.i)...)
# TODO(PR): I'm not sure about this one...
# What should it *mean* for `typemax(FD) ÷ FD(0.5)` to overflow?
@eval function Base.$divfn(x::T, y::T) where {T <: FD}
C = coefficient(T)
return reinterpret(T, C * $divfn(promote(x.i, y.i)...))
end
end
if VERSION >= v"1.4.0-"
# div(x.i, y.i) eliminates the scaling coefficient, so we call the FD constructor.
# We don't need any widening logic, since we won't be multiplying by the coefficient.
Base.div(x::T, y::T, r::RoundingMode) where {T <: FD} = T(div(x.i, y.i, r))
@eval function Base.div(x::T, y::T, r::RoundingMode) where {T <: FD}
C = coefficient(T)
return reinterpret(T, C * div(x.i, y.i, r))
end
end

# --- Checked arithmetic ---

Base.checked_add(x::FD, y::FD) = Base.checked_add(promote(x, y)...)
Base.checked_sub(x::FD, y::FD) = Base.checked_sub(promote(x, y)...)
Base.checked_mul(x::FD, y::FD) = Base.checked_mul(promote(x, y)...)
Base.checked_div(x::FD, y::FD) = Base.checked_div(promote(x, y)...)
Base.checked_cld(x::FD, y::FD) = Base.checked_cld(promote(x, y)...)
Base.checked_fld(x::FD, y::FD) = Base.checked_fld(promote(x, y)...)
Base.checked_rem(x::FD, y::FD) = Base.checked_rem(promote(x, y)...)
Base.checked_mod(x::FD, y::FD) = Base.checked_mod(promote(x, y)...)

Check warning on line 392 in src/FixedPointDecimals.jl

View check run for this annotation

Codecov / codecov/patch

src/FixedPointDecimals.jl#L385-L392 

Added lines #L385 - L392 were not covered by tests

Base.checked_add(x::FD, y) = Base.checked_add(promote(x, y)...)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Also here would be good to audit if promote is a good idea when one of the inputs is a BigInt

Copy link
Member Author

Choose a reason for hiding this comment

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

Currently, I think that this package just relies on promotion to do arithmetic on BigInts, which I agree is causing unnecessary allocs:

julia> @which FD{BigInt,2}(2) + 2
+(x::Number, y::Number)
     @ Base promotion.jl:410

julia> @code_typed FD{BigInt,2}(2) + 2
CodeInfo(
1 ─ %1 = invoke Base.GMP.MPZ.set_si(10::Int64)::BigInt
│   %2 = invoke Base.GMP.bigint_pow(%1::BigInt, 2::Int64)::BigInt
│   %3 = invoke Base.GMP.MPZ.mul_si(%2::BigInt, y::Int64)::BigInt
│   %4 = Base.getfield(x, :i)::BigInt
│   %5 = invoke Base.GMP.MPZ.add(%4::BigInt, %3::BigInt)::BigInt
│   %6 = %new(FixedDecimal{BigInt, 2}, %5)::FixedDecimal{BigInt, 2}
└──      return %6
) => FixedDecimal{BigInt, 2}

julia> @code_typed optimize=false FD{BigInt,2}(2) + 2
CodeInfo(
1 ─ %1 = Base.:+::Core.Const(+)
│   %2 = Base.promote(x, y)::Tuple{FixedDecimal{BigInt, 2}, FixedDecimal{BigInt, 2}}
│   %3 = Core._apply_iterate(Base.iterate, %1, %2)::FixedDecimal{BigInt, 2}
└──      return %3
) => FixedDecimal{BigInt, 2}

I'm just going to file this as a future improvement and move on, since I feel bad about how long this PR has lagged for.

Copy link
Member Author

Choose a reason for hiding this comment

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

Filed: #87.

Base.checked_add(x, y::FD) = Base.checked_add(promote(x, y)...)
Base.checked_sub(x::FD, y) = Base.checked_sub(promote(x, y)...)
Base.checked_sub(x, y::FD) = Base.checked_sub(promote(x, y)...)
Base.checked_mul(x::FD, y) = Base.checked_mul(promote(x, y)...)
Base.checked_mul(x, y::FD) = Base.checked_mul(promote(x, y)...)
Base.checked_div(x::FD, y) = Base.checked_div(promote(x, y)...)

Check warning on line 400 in src/FixedPointDecimals.jl

View check run for this annotation

Codecov / codecov/patch

src/FixedPointDecimals.jl#L400 

Added line #L400 was not covered by tests
Base.checked_div(x, y::FD) = Base.checked_div(promote(x, y)...)
Base.checked_cld(x::FD, y) = Base.checked_cld(promote(x, y)...)
Base.checked_cld(x, y::FD) = Base.checked_cld(promote(x, y)...)
Base.checked_fld(x::FD, y) = Base.checked_fld(promote(x, y)...)
Base.checked_fld(x, y::FD) = Base.checked_fld(promote(x, y)...)
Base.checked_rem(x::FD, y) = Base.checked_rem(promote(x, y)...)
Base.checked_rem(x, y::FD) = Base.checked_rem(promote(x, y)...)
Base.checked_mod(x::FD, y) = Base.checked_mod(promote(x, y)...)
Base.checked_mod(x, y::FD) = Base.checked_mod(promote(x, y)...)

Check warning on line 409 in src/FixedPointDecimals.jl

View check run for this annotation

Codecov / codecov/patch

src/FixedPointDecimals.jl#L402-L409 

Added lines #L402 - L409 were not covered by tests

function Base.checked_add(x::T, y::T) where {T<:FD}
z, b = Base.add_with_overflow(x.i, y.i)
b && Base.Checked.throw_overflowerr_binaryop(:+, x, y)
return reinterpret(T, z)

Check warning on line 414 in src/FixedPointDecimals.jl

View check run for this annotation

Codecov / codecov/patch

src/FixedPointDecimals.jl#L414 

Added line #L414 was not covered by tests
end
NHDaly marked this conversation as resolved.
Show resolved Hide resolved
function Base.checked_sub(x::T, y::T) where {T<:FD}
z, b = Base.sub_with_overflow(x.i, y.i)
b && Base.Checked.throw_overflowerr_binaryop(:-, x, y)
return reinterpret(T, z)

Check warning on line 419 in src/FixedPointDecimals.jl

View check run for this annotation

Codecov / codecov/patch

src/FixedPointDecimals.jl#L419 

Added line #L419 was not covered by tests
end
function Base.checked_mul(x::FD{T,f}, y::FD{T,f}) where {T<:Integer,f}
powt = coefficient(FD{T, f})
quotient, remainder = fldmodinline(widemul(x.i, y.i), powt)
v = _round_to_nearest(quotient, remainder, powt)
typemin(T) <= v <= typemax(T) || Base.Checked.throw_overflowerr_binaryop(:*, x, y)
return reinterpret(FD{T, f}, T(v))

Check warning on line 426 in src/FixedPointDecimals.jl

View check run for this annotation

Codecov / codecov/patch

src/FixedPointDecimals.jl#L426 

Added line #L426 was not covered by tests
end
# Checked division functions
for divfn in [:div, :fld, :cld]
@eval function Base.$(Symbol("checked_$divfn"))(x::FD{T,f}, y::FD{T,f}) where {T<:Integer,f}
C = coefficient(FD{T, f})
# Note: The div() will already throw for divide-by-zero and typemin(T) ÷ -1.
v, b = Base.Checked.mul_with_overflow(C, $divfn(x.i, y.i))
b && _throw_overflowerr_op($(QuoteNode(divfn)), x, y)
return reinterpret(FD{T, f}, v)

Check warning on line 435 in src/FixedPointDecimals.jl

View check run for this annotation

Codecov / codecov/patch

src/FixedPointDecimals.jl#L435 

Added line #L435 was not covered by tests
end
end
for remfn in [:rem, :mod]
# rem and mod already check for divide-by-zero and typemin(T) ÷ -1, so nothing to do.
@eval Base.$(Symbol("checked_$remfn"))(x::T, y::T) where {T <: FD} = $remfn(x, y)
end

_throw_overflowerr_op(op, x::T, y::T) where T = throw(OverflowError("$op($x, $y) overflowed for type $T"))
NHDaly marked this conversation as resolved.
Show resolved Hide resolved

function Base.checked_neg(x::T) where {T<:FD}
r = -x
(x<0) & (r<0) && Base.Checked.throw_overflowerr_negation(x)
return r
end
function Base.checked_abs(x::FD)
r = ifelse(x<0, -x, x)
r<0 || return r
_throw_overflow_abs(x)
end
if VERSION >= v"1.8.0-"
@noinline _throw_overflow_abs(x) =
throw(OverflowError(LazyString("checked arithmetic: cannot compute |x| for x = ", x, "::", typeof(x))))
else
@noinline _throw_overflow_abs(x) =
throw(OverflowError("checked arithmetic: cannot compute |x| for x = $x"))
end

# We introduce a new function for this since Base.Checked only supports integers, and ints
# don't have a decimal division operation.
"""
FixedPointDecimals.checked_decimal_division(x::FD, y::FD) -> FD

Calculates `x / y`, checking for overflow errors where applicable.

The overflow protection may impose a perceptible performance penalty.

See also:
- `Base.checked_div` for truncating division.
"""
checked_decimal_division(x::FD, y::FD) = checked_decimal_division(promote(x, y)...)

Check warning on line 475 in src/FixedPointDecimals.jl

View check run for this annotation

Codecov / codecov/patch

src/FixedPointDecimals.jl#L475 

Added line #L475 was not covered by tests
checked_decimal_division(x, y::FD) = checked_decimal_division(promote(x, y)...)
checked_decimal_division(x::FD, y) = checked_decimal_division(promote(x, y)...)

Check warning on line 477 in src/FixedPointDecimals.jl

View check run for this annotation

Codecov / codecov/patch

src/FixedPointDecimals.jl#L477 

Added line #L477 was not covered by tests

function checked_decimal_division(x::FD{T,f}, y::FD{T,f}) where {T<:Integer,f}
powt = coefficient(FD{T, f})
quotient, remainder = fldmod(widemul(x.i, powt), y.i)
v = _round_to_nearest(quotient, remainder, y.i)
typemin(T) <= v <= typemax(T) || Base.Checked.throw_overflowerr_binaryop(:/, x, y)
return reinterpret(FD{T, f}, v)
end
NHDaly marked this conversation as resolved.
Show resolved Hide resolved

# --------------------------

Base.convert(::Type{AbstractFloat}, x::FD) = convert(floattype(typeof(x)), x)
function Base.convert(::Type{TF}, x::FD{T, f}) where {TF <: AbstractFloat, T, f}
convert(TF, x.i / coefficient(FD{T, f}))::TF
Expand Down
147 changes: 143 additions & 4 deletions test/FixedDecimal.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -539,7 +539,7 @@ end

# signed integers using two's complement have one additional negative value
if x < 0 && x == typemin(x)
@test_throws InexactError x / -one(x)
@test x / -one(x) == x # -typemin(x) == typemin(x)
else
@test x / -one(x) == -x
end
Expand Down Expand Up @@ -624,9 +624,97 @@ end
@test FD{Int8,1}(2) / Int8(20) == FD{Int8,1}(0.1)
end

@testset "limits" begin
@test_throws InexactError Int8(1) / FD{Int8,2}(0.4)
@test_throws InexactError FD{Int8,2}(1) / FD{Int8,2}(0.4)
@testset "limits: overflow" begin
# Easy to reason about cases of overflow:
@test_throws OverflowError Base.checked_add(FD{Int8,2}(1), FD{Int8,2}(1))
@test_throws OverflowError Base.checked_add(FD{Int8,2}(1), 1)
@test_throws OverflowError Base.checked_add(FD{Int8,2}(1), FD{Int8,2}(0.4))

@test_throws OverflowError Base.checked_sub(FD{Int8,2}(1), FD{Int8,2}(-1))
@test_throws OverflowError Base.checked_sub(1, FD{Int8,2}(-1))
@test_throws OverflowError Base.checked_sub(FD{Int8,2}(-1), FD{Int8,2}(0.4))

@test_throws OverflowError Base.checked_mul(FD{Int8,2}(1.2), FD{Int8,2}(1.2))
@test_throws OverflowError Base.checked_mul(FD{Int8,1}(12), 2)
@test_throws OverflowError Base.checked_mul(FD{Int8,0}(120), 2)
@test_throws OverflowError Base.checked_mul(120, FD{Int8,0}(2))

@test_throws OverflowError Base.checked_div(FD{Int8,2}(1), FD{Int8,2}(0.5))
@test_throws OverflowError Base.checked_div(1, FD{Int8,2}(0.5))
@test_throws OverflowError Base.checked_div(FD{Int8,2}(1), FD{Int8,2}(0.4))

@testset "checked_decimal_division" begin
using FixedPointDecimals: checked_decimal_division

@test checked_decimal_division(Int8(1), FD{Int8,2}(0.8)) == FD{Int8,2}(1.25)
@test_throws OverflowError checked_decimal_division(Int8(1), FD{Int8,2}(0.7))
end

# Rounds down to -2
@test_throws OverflowError Base.checked_fld(FD{Int8,2}(-1), FD{Int8,2}(0.9))
# Rounds up to 2
@test_throws OverflowError Base.checked_cld(FD{Int8,2}(1), FD{Int8,2}(0.9))

# Rem and Mod only throw DivideError and nothing more. They can't overflow, since
# they can only return smaller values than the arguments.
@test_throws DivideError Base.checked_rem(FD{Int8,2}(-1), FD{Int8,2}(0))
@test_throws DivideError Base.checked_mod(FD{Int8,2}(-1), FD{Int8,2}(0))

@test_throws OverflowError Base.checked_abs(typemin(FD{Int8,2}))
@test_throws OverflowError Base.checked_neg(typemin(FD{Int8,2}))
@test Base.checked_abs(typemax(FD{Int8,2})) == FD{Int8,2}(1.27)
@test Base.checked_neg(typemax(FD{Int8,2})) == FD{Int8,2}(-1.27)

@testset "Overflow corner cases" begin
@testset for I in (Int128, UInt128, Int8, UInt8), f in (0,2)
T = FD{I, f}
issigned(I) = signed(I) === I

@test_throws OverflowError Base.checked_add(typemax(T), eps(T))
issigned(I) && @test_throws OverflowError Base.checked_add(typemin(T), -eps(T))
@test_throws OverflowError Base.checked_add(typemax(T), 1)
@test_throws OverflowError Base.checked_add(1, typemax(T))

@test_throws OverflowError Base.checked_sub(typemin(T), eps(T))
issigned(I) && @test_throws OverflowError Base.checked_sub(typemax(T), -eps(T))
@test_throws OverflowError Base.checked_sub(typemin(T), 1)
if issigned(I) && 2.0 <= typemax(T)
@test_throws OverflowError Base.checked_sub(-2, typemax(T))
end

@test_throws OverflowError Base.checked_mul(typemax(T), typemax(T))
issigned(I) && @test_throws OverflowError Base.checked_mul(typemin(T), typemax(T))
if 2.0 <= typemax(T)
@test_throws OverflowError Base.checked_mul(typemax(T), 2)
@test_throws OverflowError Base.checked_mul(2, typemax(T))
issigned(I) && @test_throws OverflowError Base.checked_mul(typemin(T), 2)
issigned(I) && @test_throws OverflowError Base.checked_mul(2, typemin(T))
end

if f > 0
@test_throws OverflowError Base.checked_div(typemax(T), eps(T))
issigned(I) && @test_throws OverflowError Base.checked_div(typemin(T), eps(T))
issigned(I) && @test_throws OverflowError Base.checked_div(typemax(T), -eps(T))

issigned(I) && @test_throws DivideError Base.checked_div(typemax(T), T(0))
issigned(I) && @test_throws DivideError Base.checked_div(typemin(T), T(0))
issigned(I) && @test_throws DivideError Base.checked_div(typemin(T), -eps(T))
end

if f > 0
@test_throws OverflowError Base.checked_fld(typemax(T), eps(T))
issigned(I) && @test_throws OverflowError Base.checked_fld(typemin(T), eps(T))
issigned(I) && @test_throws OverflowError Base.checked_fld(typemax(T), -eps(T))

@test_throws OverflowError Base.checked_cld(typemax(T), eps(T))
issigned(I) && @test_throws OverflowError Base.checked_cld(typemin(T), eps(T))
issigned(I) && @test_throws OverflowError Base.checked_cld(typemax(T), -eps(T))
end

issigned(I) && @test_throws OverflowError Base.checked_abs(typemin(T))
issigned(I) && @test_throws OverflowError Base.checked_neg(typemin(T))
end
end
end

@testset "limits of $T" for T in CONTAINER_TYPES
Expand Down Expand Up @@ -712,6 +800,57 @@ end
end
end

@testset "overflow" begin
T = FD{Int8, 1}
@testset "addition" begin
@test typemax(T) + eps(T) == typemin(T)
@test typemin(T) + (-eps(T)) == typemax(T)
end

@testset "subtraction" begin
@test typemin(T) - eps(T) == typemax(T)
@test typemax(T) - (-eps(T)) == typemin(T)
end

@testset "multiplication" begin
@test typemax(T) * 2 == T(-0.2)
@test typemin(T) * 2 == T(0)
end

@testset "division" begin
# TODO(PR): Is this the expected value?
@test typemax(T) / T(0.5) == FD2(-0.2)
@test typemin(T) / T(0.5) == FD2(0)
end

@testset "truncating division" begin
# TODO(PR): Is this the expected value?
@test typemax(T) ÷ T(0.5) == T(-0.6)
@test typemin(T) ÷ T(0.5) == T(0.6)
@test typemax(T) ÷ eps(T) == T(-1)
@test typemin(T) ÷ eps(T) == T(0)
end

@testset "fld / cld" begin
# TODO(PR): Is this the expected value?
@test fld(typemax(T), T(0.5)) == T(-0.6)
@test fld(typemin(T), T(0.5)) == T(-0.4)
@test fld(typemax(T), eps(T)) == T(-1)
@test fld(typemin(T), eps(T)) == T(0)

# TODO(PR): Is this the expected value?
@test cld(typemax(T), T(0.5)) == T(0.4)
@test cld(typemin(T), T(0.5)) == T(0.6)
@test cld(typemax(T), eps(T)) == T(-1)
@test cld(typemin(T), eps(T)) == T(0)
end
Copy link
Member Author

@NHDaly NHDaly Dec 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

(Sorry, reposting the question since I edited the tests to be FD{Int8,1} instead of FD{Int,2}):

@Drvi / @mcmcgrath13 / @omus: This is the last open question in this PR I think: What should the value of overflowing division and truncating division be?

I think that they should be the same, only differing in their rounding modes, but currently they are not.
Ideally x ÷ y would be the same as round(5 / 6, RoundToZero), which I think it currently is without overflow, but it certainly is not after overflow.
In particular, I think that trunc-divide should always return a whole-number, even if the operation overflowed?

Gosh, or maybe we should just leave all the division operators always throwing and never wrapping?? It's complicated!

What I have done so far in this PR is: trunc-divide the inner integers (so div(typemax(Int8), 5), in this case), then multiplied that by C (10), which overflows, and then I left that overflow alone:

julia> typemax(Int8) ÷ Int8(5)
25

julia> (typemax(Int8) ÷ Int8(5)) * Int8(10)
-6

But now I actually think that the right thing to do is to perform nontruncating division, and then truncate the result??
So typemax(FD{Int8,1}) ÷ FD{Int8,1}(0.5) would be 0 (since -0.2 rounds to 0) and fld would be -1?

What do you all think?

Thanks!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, this is tricky. I find it hard to even define criteria by which I'd evaluate the different approaches, because the result of the overflowing operation is probably not useful no matter how hard one tries to define its sematics.

But if I think about overflows in multiplication / addition, here is roughtly what I expect
a) Behind the scenes, a "correct number" is produced
b) If the "correct number" is too big for the storage type, it wraps around

I'm not sure how economical is to produce the "correct number" only for it wrap around, maybe there are ways to speed things up at the cost of some UB (and maybe there is a place for unsafe_div, not sure), but I think it makes sense and provides a reasonable mental model for diagnosing weird results. So for example:

julia> div(FD{Int8,2}(0.5), FD{Int8,2}(0.33)) # (50 / 33) * 100 = 151.515... (overflows max of 127) -> round to 100 (no longer overflows) -> convert to FD (% Int8, no change)
FixedDecimal{Int8,2}(1.00)

julia> div(FD{Int8,2}(0.5), FD{Int8,2}(0.2))  # (50 / 20) * 100 = 250 (overflows max of 127) -> round to 200 (still overflows) -> convert to FD (% Int8, we get -56)
FixedDecimal{Int8,2}(-0.56)

in your example:

typemax(FD{Int8,1}) ÷ FD{Int8,1}(0.5) # (127 / 5) * 10 = 254 (overflows max of 127) -> round to 250 (still overflows) -> convert to FD (% Int8, we get -6)
FixedDecimal{Int8,1}(-0.6)

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, it's so tricky! :/ I agree, i'm not even sure if overflow makes sense for truncating division.

I like your formalization, thanks. I think it's quite similar to what the code is currently doing, which is why we're getting -0.6. 👍

But the more that I think about it, i'm wondering about swapping the order of the last two operations?:

  • You wrote:
    • (127 / 5) * 10 = 254 (overflows max of 127) -> round to 250 (still overflows) -> convert to FD (% Int8, we get -6)
    • (50 / 20) * 100 = 250 (overflows max of 127) -> round to 200 (still overflows) -> convert to FD (% Int8, we get -56)
    • (50 / 33) * 100 = 151.515... (overflows max of 127) -> round to 100 (no longer overflows) -> convert to FD (% Int8, no change)
  • Whereas I think I'd like to do this:
    • (127 / 5) * 10 = 254 (overflows max of 127) -> convert to FD (% Int8, gives -2) -> round `-0.2` to `-0`.
    • (50 / 20) * 100 = 250 (overflows max of 127) -> convert to FD (% Int8, gives -6) -> round `-0.6` to `-0`.
    • (50 / 33) * 100 = 151.515... (overflows max of 127) -> convert to FD (% Int8, gives -105) -> round `-1.05` to `1.`.

This way, we preserve the invariant that div is supposed to drop the fractional part of the division. From the docs:

help?> div
search: div divrem DivideError splitdrive code_native @code_native

  div(x, y)
  ÷(x, y)

  The quotient from Euclidean (integer) division. Generally equivalent to a mathematical operation x/y without a fractional part.

  See also: cld, fld, rem, divrem.

So I think any div() implementation that can return a fractional result is wrong. It seems like it should always be safe to do Int(div(x, y))?

Copy link
Member Author

Choose a reason for hiding this comment

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

But I do wonder about just throwing in this case instead 😅

Copy link
Member Author

@NHDaly NHDaly Dec 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I also like that I think my approach gives the same answer as / before the truncation, which is what I think I'd expect. It's just the truncating version of /, and they both overflow in the same way.

Copy link

Choose a reason for hiding this comment

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

I'm not very well informed in this area, but I think I'd expect

So I think any div() implementation that can return a fractional result is wrong. It seems like it should always be safe to do Int(div(x, y))?

to be true. Especially since the docs say that div will drop the fractional part, right?

Copy link
Collaborator

@Drvi Drvi Dec 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Hmm.
One issue here is that we are combining

  • wrap-around behavior which is an "integral" phenomenon
  • truncation to an integer which is a "fractional-number" phenomenon

The former is the limitation of the storage type and is an implementation detail for a FD. There is no precedence for floating point numbers to manifest wrap-around behavior (they just increase scale and eventually call it a day an Inf), while for fixed point... who knows? For floating point arithmetic IIUC, the mental model of "produce the correct number behind the scenes and then round to nearest representable" does capture their semantics.

Which brings me to another point which can help us decide: what is truncating division? Is it a single, atomic operation? Or a combination of two separate operations (the / and the trunc)? For a) it would make sense to wrap at the end, for b) it would make sense to do what you suggest. One argument for a) is that the user can always implement b) by composing the two operations manually (but maybe this would be confusing to users? Freedom for one user is possible confusion for the other...).

I guess the nearest sibling FixedDecimals have in this regard are Rationals, which also use integers to implement fractional numbers. They apparently recognize this is not a well defined situation an throw:

julia> Rational{Int8}(127, 1) // Rational{Int8}(1, 2)
ERROR: OverflowError: 127 * 2 overflowed for type Int8

Note that an alternative to fixed point decimals are floating point decimals (e.g. https://github.com/JuliaMath/DecFP.jl). IIUC, they could be a viable substitute for fixed point decimal and they have some advantages too:

  • They are well-defined by a IEEE standard
  • They have one fewer type param
  • Being a floating point, they're able to adapt to what your scale is (and if you are not surpassing the number of significant digits within that scale, they are precise, I think)

But...:

  • Their precision is lower than for fixed decimals (7, 16, and 34 digits for 32, 64 and 128 bits respectively)

Copy link
Member Author

Choose a reason for hiding this comment

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

OKAY, following the behavior of Rational, I have changed this PR to always throw OverflowError on overflow during division operations.

With this, the tests are passing, and i think this PR is finally ready to go! Thanks for the great discussion.

I'll leave this thread open here for anyone who reads the PR in the future.


@testset "abs / neg" begin
@test abs(typemin(T)) == typemin(T)
@test -(typemin(T)) == typemin(T)
end
end

@testset "isinteger" begin
# Note: Test cannot be used unless we can construct `FD{Int8,6}`
# @testset "overflow" begin
Expand Down
Loading