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

Missing RoundFromZero support for three-arg div with RoundingMode #34519

Closed
NHDaly opened this issue Jan 26, 2020 · 12 comments
Closed

Missing RoundFromZero support for three-arg div with RoundingMode #34519

NHDaly opened this issue Jan 26, 2020 · 12 comments
Labels
feature Indicates new feature / enhancement requests good first issue Indicates a good issue for first-time contributors to Julia maths Mathematical functions

Comments

@NHDaly
Copy link
Member

NHDaly commented Jan 26, 2020

julia> div(2,3, RoundFromZero)
ERROR: MethodError: no method matching div(::Int64, ::Int64, ::RoundingMode{:FromZero})
Closest candidates are:
  div(::Real, ::Real, ::RoundingMode) at div.jl:247
  div(::Missing, ::Number, ::RoundingMode) at missing.jl:121
  div(::T, ::T) where T<:Union{Int16, Int32, Int64, Int8} at int.jl:230
  ...
Stacktrace:
 [1] div(::Int64, ::Int64, ::RoundingMode{:FromZero}) at ./div.jl:247
 [2] top-level scope at REPL[6]:1

It looks like maybe this rounding mode was missed when support for three-arg div was added in #33040?:
https://github.com/JuliaLang/julia/pull/33040/files#diff-b1fc87557f78879f66387487c7a02689R203-R213

@NHDaly
Copy link
Member Author

NHDaly commented Jan 26, 2020

CC: @Keno

@JeffreySarnoff
Copy link
Contributor

good catch

@sostock
Copy link
Contributor

sostock commented Apr 4, 2020

Note that the RoundFromZero docstring states that this rounding mode is only defined for BigFloat.

@JeffreySarnoff
Copy link
Contributor

We can edit the docstring to reflect additional type coverage, when available.

@rapus95
Copy link
Contributor

rapus95 commented Jun 7, 2020

Would that do it?

function Base.div(x::T, y::T, ::typeof(RoundFromZero)) where T<:Integer
           sx = sign(x)
           sy = sign(y)
           d = div(sx*x, sy*y, RoundUp)
           return sx*sy*d
       end

It might even be possible to drop the restriction to Integer entirely.

Edit: won't work for typemin :/
Edit2: how about flipping everything to negative and using RoundDown? I.e.

function Base.div(x::T, y::T, ::typeof(RoundFromZero)) where T<:Integer
           sx = sign(x)
           sy = sign(y)
           d = div(-sx*x, -sy*y, RoundUp)
           return sx*sy*d
       end

@JeffreySarnoff
Copy link
Contributor

JeffreySarnoff commented Jun 8, 2020

perhaps

roundtozero(x,y) = ifelse(signbit(x)===signbit(y), RoundUp, RoundDown)
# or
# roundtozero(x,y) = signbit(x) === signbit(y) ? RoundUp : RoundDown

Base.div(x::T, y::T, ::typeof(RoundFromZero)) where {T<:Integer} =
    let rounding_direction = roundtozero(x,y)
       div(x, y, rounding_direction)
    end

@rapus95
Copy link
Contributor

rapus95 commented Jun 8, 2020

why is that let block important & do we really have to make that ifelse an external function?

@JeffreySarnoff
Copy link
Contributor

JeffreySarnoff commented Jun 8, 2020

There is no need to make the ifelse external .. I did that because it kept the code clearer, cleaner to me, as I worked through variations. I found the let block ran somewhat faster (5%). It is not necessary, though, and when broadcasting over vectors this version is just as fast.

function Base.div(x::T, y::T, ::typeof(RoundFromZero)) where {T<:Integer}
    rounddir = ifelse(signbit(x) === signbit(y), RoundUp, RoundDown)
    return div(x, y, rounddir)
end

@NHDaly NHDaly added feature Indicates new feature / enhancement requests good first issue Indicates a good issue for first-time contributors to Julia maths Mathematical functions labels Dec 20, 2020
@jessymilare
Copy link
Contributor

jessymilare commented Jun 6, 2021

Why doesn't Julia include support for RoundFromZero for all floating point types as well? I've made some tests with the following simple code:

function Base.round(x::AbstractFloat, ::typeof(RoundFromZero))
    y = trunc(x)
    ifelse(x == y, y, y + sign(y))
end

When compiled with "-O3", it seems quite fast (only around 40% slower than RoundToZero 15% slower than RoundNearestTiesUp):

julia> v = rand(Float64, 10000) .* 200;

julia> using BenchmarkTools

julia> w = similar(v)

julia> @benchmark w .= round.(v, Ref(RoundFromZero))
BenchmarkTools.Trial: 
  memory estimate:  72 bytes
  allocs estimate:  3
  --------------
  minimum time:     3.521 μs (0.00% GC)
  median time:      3.628 μs (0.00% GC)
  mean time:        3.690 μs (0.00% GC)
  maximum time:     7.849 μs (0.00% GC)
  --------------
  samples:          10000
  evals/sample:     8

julia> @benchmark w .= round.(v, Ref(RoundToZero))
BenchmarkTools.Trial: 
  memory estimate:  72 bytes
  allocs estimate:  3
  --------------
  minimum time:     2.333 μs (0.00% GC)
  median time:      2.376 μs (0.00% GC)
  mean time:        2.422 μs (0.00% GC)
  maximum time:     84.613 μs (0.00% GC)
  --------------
  samples:          10000
  evals/sample:     9

julia> @benchmark w .= round.(v, Ref(RoundNearestTiesUp))
BenchmarkTools.Trial: 
  memory estimate:  72 bytes
  allocs estimate:  3
  --------------
  minimum time:     2.775 μs (0.00% GC)
  median time:      2.840 μs (0.00% GC)
  mean time:        2.871 μs (0.00% GC)
  maximum time:     8.655 μs (0.00% GC)
  --------------
  samples:          10000
  evals/sample:     9

Maybe, if this method was compiled into Julia, it could be as fast as the other rounding methods.

@jessymilare
Copy link
Contributor

Tried this other version and it runs faster (performance similar to RoundNearestTiesUp):

function Base.round(x::AbstractFloat, ::typeof(RoundFromZero))
    round(x, ifelse(signbit(x), RoundDown, RoundUp))
end
ulia> @benchmark w .= round.(v, Ref(RoundFromZero))
BenchmarkTools.Trial: 
  memory estimate:  72 bytes
  allocs estimate:  3
  --------------
  minimum time:     2.708 μs (0.00% GC)
  median time:      2.766 μs (0.00% GC)
  mean time:        2.801 μs (0.00% GC)
  maximum time:     8.674 μs (0.00% GC)
  --------------
  samples:          10000
  evals/sample:     9

@JeffreySarnoff
Copy link
Contributor

@admin would you create a new issue by splitting off the previous question @jessymilare ? Her question is a good one that we have discussed in the past, although I found no issue about adding support for RoundFromZero as a floating point rounding mode.

@vtjnash
Copy link
Sponsor Member

vtjnash commented Feb 1, 2024

Fixed by #41246

@vtjnash vtjnash closed this as completed Feb 1, 2024
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 good first issue Indicates a good issue for first-time contributors to Julia maths Mathematical functions
Projects
None yet
Development

No branches or pull requests

6 participants