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

remove test for empty tuple for mean #36

Merged
merged 1 commit into from
May 15, 2020
Merged

remove test for empty tuple for mean #36

merged 1 commit into from
May 15, 2020

Conversation

KristofferC
Copy link
Member

JuliaLang/julia#35803 broke the tests for Statistics because an ambiguity was fixed:

julia> using Statistics

julia> mean(())
ERROR: MethodError: reduce_empty(::typeof(Base.add_sum), ::Type{Union{}}) is ambiguous. Candidates:
  reduce_empty(::typeof(Base.add_sum), ::Type{T}) where T<:Union{Int16, Int32, Int8} in Base at reduce.jl:314
  reduce_empty(::typeof(Base.add_sum), ::Type{T}) where T<:Union{UInt16, UInt32, UInt8} in Base at reduce.jl:315
Possible fix, define
  reduce_empty(::typeof(Base.add_sum), ::Type{Union{}})

which after that PR is now (more correctly):

julia> mean(())
ERROR: ArgumentError: reducing over an empty collection is not allowed

This test doesn't seem to do anything so just delete it.

@timholy
Copy link
Contributor

timholy commented May 8, 2020

Without that PR it's

julia> mean(())
ERROR: MethodError: zero(::Type{Union{}}) is ambiguous. Candidates:
  zero(::Type{var"#s822"} where var"#s822"<:AbstractIrrational) in Base at irrationals.jl:149
  zero(::Union{Type{P}, P}) where P<:Dates.Period in Dates at /home/tim/src/julia-master/usr/share/julia/stdlib/v1.6/Dates/src/periods.jl:53
  zero(::Type{T}) where T<:Number in Base at number.jl:242
Possible fix, define
  zero(::Type{Union{}})
Stacktrace:
 [1] reduce_empty(::typeof(+), ::Type{T} where T) at ./reduce.jl:310
 [2] mapreduce_empty(::typeof(identity), ::Function, ::Type{T} where T) at ./reduce.jl:339
 [3] reduce_empty(::Base.MappingRF{typeof(identity),typeof(+)}, ::Type{T} where T) at ./reduce.jl:325
 [4] reduce_empty_iter at ./reduce.jl:351 [inlined]
 [5] mapreduce_empty_iter(::Function, ::Function, ::Tuple{}, ::Base.HasEltype) at ./reduce.jl:347
 [6] mean(::typeof(identity), ::Tuple{}) at /home/tim/src/julia-master/usr/share/julia/stdlib/v1.6/Statistics/src/Statistics.jl:64
 [7] mean(::Tuple{}) at /home/tim/src/julia-master/usr/share/julia/stdlib/v1.6/Statistics/src/Statistics.jl:44
 [8] top-level scope at REPL[3]:1

This might be a consequence of

julia> Base.IteratorEltype(())
Base.HasEltype()

I think for Julia 1.6 we should explore

diff --git a/base/generator.jl b/base/generator.jl
index 7cd075d255..cfe520070f 100644
--- a/base/generator.jl
+++ b/base/generator.jl
@@ -126,3 +126,6 @@ IteratorEltype(::Type) = HasEltype()  # HasEltype is the default
 IteratorEltype(::Type{Generator{I,T}}) where {I,T} = EltypeUnknown()
 
 IteratorEltype(::Type{Any}) = EltypeUnknown()
+
+IteratorEltype(::Type{<:Tuple}) = EltypeUnknown()
+IteratorEltype(::Type{Tuple{T,Vararg{T}}}) where {T} = HasEltype()

Running local tests now to see what breaks.

@tkf
Copy link
Contributor

tkf commented May 9, 2020

👍 for this change

I think reduce_empty should handle Union{} correctly. So, a part of JuliaLang/julia#35733 does make sense even only from the point of view of correctness. For example, sum(Union{}[]) should not throw MethodError.

@tkf
Copy link
Contributor

tkf commented May 12, 2020

This is required for JuliaLang/julia#35843

@nalimilan
Copy link
Member

Sorry for not responding before, but can you add a test for the new behavior conditional on the version? That sounds like a legitimate case to test.

@tkf
Copy link
Contributor

tkf commented May 15, 2020

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

Successfully merging this pull request may close these issues.

4 participants