Skip to content

Commit

Permalink
Update round(::Missing) to match float version (#30971)
Browse files Browse the repository at this point in the history
* Update round(::Missing) to match float version

`round()` was updated to accept `digits` and `base` as keyword arguments. It looks like the `Missing` version was... missed? 😈
This prevents e.g. `round(A, digits=4)` and `round(A, 4)` (old signature) from working on `Union{Missing, Float64}` arrays `A`.

* Fix tests for updated round(::Missing, ...)

* Test elementwise rounding on Union{T, Missing}[...]

When the signature of `Base.round()` and friends (ceil,floor,trunc) was changed to accept keyword arguments for `digits` and `base` instead of positional arguments, the implementations for `Missing` were missed. Test these functions elementwise on arrays of Floats and Missing values to make sure their signatures are compatible with the versions for numbers.

* Only accept Integer base in rounding

* Cover missing Missing variants of round() etc.

Accept `sigdigits` keyword argument in addition to `digits` and `base`. Also accept `RoundingMode` positional argument.

* More variants for round(::Missing) tests

Test that there are Missing versions of remaining variants of round() and friends.

* Remove extraneous kwargs from trunc,floor,ceil

* Clean up trunc,floor,ceil defs for Missing

Continue to call `round()`, despite it just returning `missing`, so that errors happen with kwargs or types that are invalid.

* Pass RoundingMode to round() in Missing versions

* Check RoundingMode is being passed on for Missing

* Use isequal and ===

* Test round() with sigdigits on missing-value arrays

Also, separate tests for digits and sigdigits: Combining these arguments is not valid for floats, although it currently goes through for Missing. Finally, use a sensible base for the digits and sigdigits tests.

* Fix new round() ambiguities with Missing and Rational

* Reduce Rational pollution of round() methods

The specification to `RoundingMode`s made it difficult to define `round()` for `Missing`.

* Remove now-unneeded Missing,Rational methods

* Simplify Missing rounding tests

Remove duplicate tests but still test elementwise to help ensure we stay in sync with Float methods in the future. Add some comments to explain what is going on.

* Remove redundant rounded_array

`isequal(test_array, rounded_array) == true`, so just use `test_array`.

* Remove Missing, Rational specializations

My (possibly incomplete) tests suggest these are no longer needed.

* Restore Rational, Missing rounding functions

Still needed these to avoid ambiguities after all.
  • Loading branch information
amilsted authored and ararslan committed Feb 19, 2019
1 parent 52bafeb commit f0d88dd
Show file tree
Hide file tree
Showing 3 changed files with 33 additions and 22 deletions.
15 changes: 12 additions & 3 deletions base/missing.jl
Original file line number Diff line number Diff line change
Expand Up @@ -107,16 +107,25 @@ max(::Missing, ::Any) = missing
max(::Any, ::Missing) = missing

# Rounding and related functions
for f in (:(ceil), :(floor), :(round), :(trunc))
round(::Missing, ::RoundingMode=RoundNearest; sigdigits::Integer=0, digits::Integer=0, base::Integer=0) = missing
round(::Type{>:Missing}, ::Missing, ::RoundingMode=RoundNearest) = missing
round(::Type{T}, ::Missing, ::RoundingMode=RoundNearest) where {T} =
throw(MissingException("cannot convert a missing value to type $T: use Union{$T, Missing} instead"))
round(::Type{T}, x::Any, r::RoundingMode=RoundNearest) where {T>:Missing} = round(nonmissingtype(T), x, r)
# to fix ambiguities
round(::Type{T}, x::Rational, r::RoundingMode=RoundNearest) where {T>:Missing} = round(nonmissingtype(T), x, r)
round(::Type{T}, x::Rational{Bool}, r::RoundingMode=RoundNearest) where {T>:Missing} = round(nonmissingtype(T), x, r)

# Handle ceil, floor, and trunc separately as they have no RoundingMode argument
for f in (:(ceil), :(floor), :(trunc))
@eval begin
($f)(::Missing, digits::Integer=0, base::Integer=0) = missing
($f)(::Missing; sigdigits::Integer=0, digits::Integer=0, base::Integer=0) = missing
($f)(::Type{>:Missing}, ::Missing) = missing
($f)(::Type{T}, ::Missing) where {T} =
throw(MissingException("cannot convert a missing value to type $T: use Union{$T, Missing} instead"))
($f)(::Type{T}, x::Any) where {T>:Missing} = $f(nonmissingtype(T), x)
# to fix ambiguities
($f)(::Type{T}, x::Rational) where {T>:Missing} = $f(nonmissingtype(T), x)
($f)(::Type{T}, x::Rational{Bool}) where {T>:Missing} = $f(nonmissingtype(T), x)
end
end

Expand Down
17 changes: 5 additions & 12 deletions base/rational.jl
Original file line number Diff line number Diff line change
Expand Up @@ -368,9 +368,9 @@ end
trunc(::Type{T}, x::Rational) where {T} = convert(T,div(x.num,x.den))
floor(::Type{T}, x::Rational) where {T} = convert(T,fld(x.num,x.den))
ceil(::Type{T}, x::Rational) where {T} = convert(T,cld(x.num,x.den))
round(::Type{T}, x::Rational, r::RoundingMode=RoundNearest) where {T} = _round_rational(T, x, r)


function round(::Type{T}, x::Rational{Tr}, ::RoundingMode{:Nearest}) where {T,Tr}
function _round_rational(::Type{T}, x::Rational{Tr}, ::RoundingMode{:Nearest}) where {T,Tr}
if denominator(x) == zero(Tr) && T <: Integer
throw(DivideError())
elseif denominator(x) == zero(Tr)
Expand All @@ -384,9 +384,7 @@ function round(::Type{T}, x::Rational{Tr}, ::RoundingMode{:Nearest}) where {T,Tr
convert(T, s)
end

round(::Type{T}, x::Rational) where {T} = round(T, x, RoundNearest)

function round(::Type{T}, x::Rational{Tr}, ::RoundingMode{:NearestTiesAway}) where {T,Tr}
function _round_rational(::Type{T}, x::Rational{Tr}, ::RoundingMode{:NearestTiesAway}) where {T,Tr}
if denominator(x) == zero(Tr) && T <: Integer
throw(DivideError())
elseif denominator(x) == zero(Tr)
Expand All @@ -400,7 +398,7 @@ function round(::Type{T}, x::Rational{Tr}, ::RoundingMode{:NearestTiesAway}) whe
convert(T, s)
end

function round(::Type{T}, x::Rational{Tr}, ::RoundingMode{:NearestTiesUp}) where {T,Tr}
function _round_rational(::Type{T}, x::Rational{Tr}, ::RoundingMode{:NearestTiesUp}) where {T,Tr}
if denominator(x) == zero(Tr) && T <: Integer
throw(DivideError())
elseif denominator(x) == zero(Tr)
Expand All @@ -414,18 +412,13 @@ function round(::Type{T}, x::Rational{Tr}, ::RoundingMode{:NearestTiesUp}) where
convert(T, s)
end

function round(::Type{T}, x::Rational{Bool}) where T
function round(::Type{T}, x::Rational{Bool}, ::RoundingMode=RoundNearest) where T
if denominator(x) == false && (T <: Union{Integer, Bool})
throw(DivideError())
end
convert(T, x)
end

round(::Type{T}, x::Rational{Bool}, ::RoundingMode{:Nearest}) where {T} = round(T, x)
round(::Type{T}, x::Rational{Bool}, ::RoundingMode{:NearestTiesAway}) where {T} = round(T, x)
round(::Type{T}, x::Rational{Bool}, ::RoundingMode{:NearestTiesUp}) where {T} = round(T, x)
round(::Type{T}, x::Rational{Bool}, ::RoundingMode) where {T} = round(T, x)

trunc(x::Rational{T}) where {T} = Rational(trunc(T,x))
floor(x::Rational{T}) where {T} = Rational(floor(T,x))
ceil(x::Rational{T}) where {T} = Rational(ceil(T,x))
Expand Down
23 changes: 16 additions & 7 deletions test/missing.jl
Original file line number Diff line number Diff line change
Expand Up @@ -188,16 +188,25 @@ Base.one(::Type{Unit}) = 1
end

@testset "rounding functions" begin
rounding_functions = [ceil, floor, round, trunc]

# All rounding functions return missing when evaluating missing as first argument

# Check that the RoundingMode argument is passed on correctly
@test round(Union{Int, Missing}, 0.9) === round(Int, 0.9)
@test round(Union{Int, Missing}, 0.9, RoundToZero) === round(Int, 0.9, RoundToZero)

# Test elementwise on mixed arrays to ensure signature of Missing methods matches that of Float methods
test_array = [1.0, missing]

@test isequal(round.(test_array, RoundNearest), test_array)
@test isequal(round.(Union{Int, Missing}, test_array, RoundNearest), test_array)

rounding_functions = [ceil, floor, round, trunc]
for f in rounding_functions
@test ismissing(f(missing))
@test ismissing(f(missing, 1))
@test ismissing(f(missing, 1, 1))
@test ismissing(f(Union{Int, Missing}, missing))
@test f(Union{Int, Missing}, 1.0) === 1
@test_throws MissingException f(Int, missing)
@test isequal(f.(test_array), test_array)
@test isequal(f.(test_array, digits=0, base=10), test_array)
@test isequal(f.(test_array, sigdigits=1, base=10), test_array)
@test isequal(f.(Union{Int, Missing}, test_array), test_array)
end
end

Expand Down

0 comments on commit f0d88dd

Please sign in to comment.