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

Another extreme bug #12

Closed
dpsanders opened this issue Jun 30, 2017 · 15 comments
Closed

Another extreme bug #12

dpsanders opened this issue Jun 30, 2017 · 15 comments

Comments

@dpsanders
Copy link
Contributor

julia 0.6> FastRounding.add_round(prevfloat(Inf), prevfloat(Inf), RoundDown)
Inf

This should give prevfloat(Inf).

@dpsanders
Copy link
Contributor Author

dpsanders commented Jun 30, 2017

This is a bug in ErrorfreeArithmetic This is due to the output of ErrorfreeArithmetic:

julia 0.6> ErrorfreeArithmetic.add_errorfree(prevfloat(Inf), prevfloat(Inf))
(Inf, NaN)

@dpsanders
Copy link
Contributor Author

Seems like this will be a bug whenever the sum is Inf.

Found by @Sacha0

@dpsanders
Copy link
Contributor Author

The bug is in FastRounding, not ErrorfreeArithmetic, which gives the correct answer.
All operations in FastRounding must check if the result is Inf or -Inf, and round appropriately.

@JeffreySarnoff
Copy link
Owner

is the only issue with this family of expressions?

setrounding(Float64, RoundDown) do; prevfloat(Inf) + prevfloat(Inf); end
1.7976931348623157e308
setrounding(Float64, RoundUp) do; nextfloat(-Inf) + nextfloat(-Inf); end
-1.7976931348623157e308
setrounding(Float64, RoundUp) do; nextfloat(-Inf) - prevfloat(Inf); end
-1.7976931348623157e308
# etc

@dpsanders
Copy link
Contributor Author

dpsanders commented Jun 30, 2017

The problem is with any operation that produces an infinity, e.g.

julia 0.6> add_round(prevfloat(Inf), 1e300, RoundDown)
Inf

@JeffreySarnoff
Copy link
Owner

JeffreySarnoff commented Jun 30, 2017 via email

@dpsanders
Copy link
Contributor Author

dpsanders commented Jun 30, 2017 via email

@Sacha0
Copy link

Sacha0 commented Jun 30, 2017

More generally, for consistency with the IEEE 754 directed rounding modes? (Inferring behavior from experimentation at the REPL, haven't read the specification itself.) Best!

julia> setrounding(Float64, RoundDown)
0

julia> prevfloat(Inf) + 1.0
1.7976931348623157e308

julia> setrounding(Float64, RoundUp)
0

julia> nextfloat(-Inf) - 1.0
-1.7976931348623157e308

@JeffreySarnoff
Copy link
Owner

JeffreySarnoff commented Jun 30, 2017

helpful -- I just want the fix to stay out of the way most of the time
some Benchmarking of alternatives is in order -- will post findings

@JeffreySarnoff
Copy link
Owner

JeffreySarnoff commented Jul 8, 2017

please confirm that this is the logic you seek

@inline function round_errorfree{T<:SysFloat}(hi::T, lo::T, ::RoundingMode{:Nearest})::T
     !isinf(hi) && return hi
     return signbit(hi) ? T(-Inf) : T(Inf)
end

@inline function round_errorfree{T<:SysFloat}(hi::T, lo::T, ::RoundingMode{:ToZero})::T
     !isinf(hi) && return signbit(hi) != signbit(lo) ? AdjacentFloats.next_nearerto_zero(hi) : hi
     return signbit(hi) ? nextfloat(T(-Inf)) : prevfloat(T(Inf))
end

@inline function round_errorfree{T<:SysFloat}(hi::T, lo::T, ::RoundingMode{:FromZero})::T
    !isinf(hi) && return signbit(hi) == signbit(lo) ? AdjacentFloats.next_awayfrom_zero(hi) : hi
    return signbit(hi) ? T(-Inf) : T(Inf)
end

@inline function round_errorfree{T<:SysFloat}(hi::T, lo::T, ::RoundingMode{:Up})::T
    !isinf(hi) && return (lo<=zero(T) || isnan(lo))  ? hi : next_float(hi)
    return signbit(hi) ? nextfloat(T(-Inf)) : T(Inf)
end

@inline function round_errorfree{T<:SysFloat}(hi::T, lo::T, ::RoundingMode{:Down})::T
    !isinf(hi) && return (lo>=zero(T) || isnan(lo))  ? hi : prev_float(hi)
    return signbit(hi) ? T(-Inf) : prevfloat(T(Inf))
end

@JeffreySarnoff
Copy link
Owner

@dpsanders just check that the way infinities are handled matches expectation -- nothing else is altered .. all I am asking is for confirmation that the request and the changes match before introducing this as higher version

@JeffreySarnoff
Copy link
Owner

I assume silence is agreement :->
I merged the prior comment's edits and have it as new version

@JeffreySarnoff
Copy link
Owner

I think the new release covers this.

@JeffreySarnoff
Copy link
Owner

closing on that basis

@dpsanders
Copy link
Contributor Author

Sorry for the delay in getting to this. Looking good!

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

No branches or pull requests

3 participants