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

Could we have a RoundingMode that doesn't throw InexactError #51069

Closed
LilithHafner opened this issue Aug 27, 2023 · 12 comments
Closed

Could we have a RoundingMode that doesn't throw InexactError #51069

LilithHafner opened this issue Aug 27, 2023 · 12 comments
Labels
feature Indicates new feature / enhancement requests

Comments

@LilithHafner
Copy link
Member

Could we also have a mode that doesn't throw InexactError, and always simply gives us the nearest element of the output type, as in

round(UInt8,    +Inf, Nearest) == 0xff
round(UInt8, +1000.0, Nearest) == 0xff
round(UInt8, -1000.0, Nearest) == 0x00
round(UInt8,    -Inf, Nearest) == 0x00

where 0xff and 0x00 are the nearest UInt8 values to +Inf, 1000.0, -1000.0 and -Inf, respectively. I can think of a lot of applications (signal processing, control systems, graphics), where you never want to have to manually deal with an InexactError, and just want to use whatever value is nearest. (That's how MATLAB converts floats to integers.)

Originally posted by @mgkuhn in #50812 (comment)

@LilithHafner LilithHafner added the feature Indicates new feature / enhancement requests label Aug 27, 2023
@nsajko
Copy link
Contributor

nsajko commented Aug 27, 2023

unsafe_trunc is similar, but currently doesn't behave as requested above. It's not clear from its docs what its behavior should be IMO:

https://docs.julialang.org/en/v1/base/math/#Base.unsafe_trunc

@nsajko
Copy link
Contributor

nsajko commented Aug 27, 2023

So perhaps this functionality should be incorporated into unsafe_trunc?

@LilithHafner
Copy link
Member Author

I think it would be appropriate to define this in a package with

struct NoThrow{R} end
Base.round(::Type{T}, x, ::RoundingMode{NoThrow{R}}) where {T, R} = 
    convert(T, clamp(round(x, RoundingMode{R}()), typemin(T), typemax(T)))
const Nearest = RoundingMode{NoThrow{:Nearest}}()

using Test

@test round(UInt8,    +Inf, Nearest) === 0xff
@test round(UInt8, +1000.0, Nearest) === 0xff
@test round(UInt8, -1000.0, Nearest) === 0x00
@test round(UInt8,    -Inf, Nearest) === 0x00

@LilithHafner
Copy link
Member Author

The behavior of unsafe_trunk is intentionally unspecified when the value is not within range to allow optimizations in its implementation.

If the value is not representable by T, an arbitrary value will be returned.

@Seelengrab
Copy link
Contributor

Seelengrab commented Aug 27, 2023

In regards to round, what is round(UInt8, NaN, Nearest)? Does it matter which exact bitpattern of NaN is encountered (we don't currently guarantee any particular one for any NaN encountered, I don't think). I can see it making sense for ceil or floor, but others likely break down easily.

@LilithHafner
Copy link
Member Author

With the implementation I gave, ERROR: InexactError: UInt8(NaN). Matlab returns 0.

In signal processing and control systems, you may not want to have to manually deal with values that are too big or too small, but a NaN should probably still be explicitly dealt with. With graphics, 0 is probably okay.

@Seelengrab
Copy link
Contributor

So it's context dependent - then I'd vote for not having this in Base, since picking a preference when Base doesn't use it internally seems odd.

@PallHaraldsson
Copy link
Contributor

then I'd vote for not having this in Base, since picking a preference when Base doesn't use it internally seems odd.

That's a good argument, but it's also good for not have rounding in Base at all? :) [In the sense: I didn't check carefully, but I'm pretty sure Julia doesn't use round itself, maybe one exception, except to implement functions it also doesn't use.]

In signal processing and control systems, you may not want to have to manually deal with values that are too big or too small, but a NaN should probably still be explicitly dealt with. With graphics, 0 is probably okay.

Don't we have packages for graphics, or fixed-point, where this belongs then? For the former also, if not same package.

@LilithHafner
Copy link
Member Author

Base primarily uses rounding to implement mathematical functions like integer division, trig, exp, and rem_pio2. If I were doing it all over, I would probably not have any of those in base.

@LilithHafner LilithHafner closed this as not planned Won't fix, can't repro, duplicate, stale Aug 30, 2023
@mgkuhn
Copy link
Contributor

mgkuhn commented Aug 30, 2023

convert(T, clamp(round(x, RoundingMode{R}()), typemin(T), typemax(T))

The problem here is that this causes duplicate overflow/clamp test branches: one in clamp and a redundant one in convert. Are we confident the compiler is able to reliably eliminate these in all cases?

[P.S.: another problem of this kind of construct, where rounding and converting are separate operations, is that it doesn't provide control over the rounding method used when converting e.g. a UInt32 into a Float32 (recall that Float32 can't represent most integers over 2^24 due to its 24-bit mantissa, i.e. rounding has to happen to the nearest valid mantissa value, not the nearest integer, and therefore the rounding algorithm needs to know the destination type.]

I still believe it makes a lot of sense to treat rounding, clamping and converting as one single basic operation, both in terms of performance, flexibility and accuracy. (Treatment of NaN during conversion seems an entirely independent issue.)

@LilithHafner
Copy link
Member Author

LilithHafner commented Aug 30, 2023

Treating all three as a single operation significantly expands the cross-product of possible inputs. Whenever possible, it is better to provide simple, generic, implementations that can compose to be as efficient and precise as optimized integrated implementations.

If we swap out unsafe_trunc for convert in the above implementation (which is safe because of the clamp, unless the input is NaN), then performance becomes quite good.

struct Clamped{R} end
Base.round(::Type{T}, x, ::RoundingMode{Clamped{R}}) where {T, R} = 
    unsafe_trunc(T, clamp(round(x, RoundingMode{R}()), typemin(T), typemax(T)))
const Nearest = RoundingMode{Clamped{:Nearest}}()

@btime round(UInt8, x, Nearest) setup=(x=rand()*1000-500)
#  1.417 ns (0 allocations: 0 bytes)

@btime Base.fptoui(UInt8, x) setup=(x=rand()*100)
#  1.416 ns (0 allocations: 0 bytes)

@LilithHafner
Copy link
Member Author

To throw on NaN would hurt performance, but to return zero on NaN inputs would be possible without performance overhead with

Base.round(::Type{UInt8}, x::Float64, ::RoundingMode{Clamped{R}}) where R = 
    ccall("llvm.fptoui.sat.i8.f64", llvmcall, UInt8, (Float64,), round(x, RoundingMode{R}()))

integrating truncation and clamping (but still not needing to integrate rounding)

All this can still be implemented in a package.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Indicates new feature / enhancement requests
Projects
None yet
Development

No branches or pull requests

5 participants