From 3f3c1bc170f8048f816f60fd5db78fddbeac2882 Mon Sep 17 00:00:00 2001 From: Jan Weidner Date: Thu, 11 Jan 2018 21:24:03 +0100 Subject: [PATCH 1/5] fix non numeric accumulate(op, v0, x) (#25506) --- base/multidimensional.jl | 2 +- test/arrayops.jl | 3 +++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/base/multidimensional.jl b/base/multidimensional.jl index d49aa104e21a4..9f90c56e451f9 100644 --- a/base/multidimensional.jl +++ b/base/multidimensional.jl @@ -689,7 +689,7 @@ end # see discussion in #18364 ... we try not to widen type of the resulting array # from cumsum or cumprod, but in some cases (+, Bool) we may not have a choice. -rcum_promote_type(op, ::Type{T}, ::Type{S}) where {T,S<:Number} = promote_op(op, T, S) +rcum_promote_type(op, ::Type{T}, ::Type{S}) where {T,S} = promote_op(op, T, S) rcum_promote_type(op, ::Type{T}) where {T<:Number} = rcum_promote_type(op, T,T) rcum_promote_type(op, ::Type{T}) where {T} = T diff --git a/test/arrayops.jl b/test/arrayops.jl index 7157ca70e3360..644418579fb3b 100644 --- a/test/arrayops.jl +++ b/test/arrayops.jl @@ -2152,6 +2152,9 @@ end op(x,y) = 2x+y @test accumulate(op, [10,20, 30]) == [10, op(10, 20), op(op(10, 20), 30)] == [10, 40, 110] @test accumulate(op, [10 20 30], 2) == [10 op(10, 20) op(op(10, 20), 30)] == [10 40 110] + + #25506 + @test accumulate((acc, x) -> acc+x[1], 0, [(1,2), (3,4), (5,6)]) == [1, 4, 9] end struct F21666{T <: Base.ArithmeticStyle} From 65dff7a4146b067308f0acfd18e6cc410c3a5f7f Mon Sep 17 00:00:00 2001 From: Jan Weidner Date: Sun, 14 Jan 2018 20:18:43 +0100 Subject: [PATCH 2/5] rcum_convert --- base/multidimensional.jl | 11 ++++++----- test/arrayops.jl | 4 ++++ 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/base/multidimensional.jl b/base/multidimensional.jl index 9f90c56e451f9..75c3d9e21336f 100644 --- a/base/multidimensional.jl +++ b/base/multidimensional.jl @@ -690,13 +690,14 @@ end # see discussion in #18364 ... we try not to widen type of the resulting array # from cumsum or cumprod, but in some cases (+, Bool) we may not have a choice. rcum_promote_type(op, ::Type{T}, ::Type{S}) where {T,S} = promote_op(op, T, S) -rcum_promote_type(op, ::Type{T}) where {T<:Number} = rcum_promote_type(op, T,T) -rcum_promote_type(op, ::Type{T}) where {T} = T +rcum_promote_type(op, ::Type{T}) where {T} = rcum_promote_type(op, T,T) # handle sums of Vector{Bool} and similar. it would be nice to handle # any AbstractArray here, but it's not clear how that would be possible rcum_promote_type(op, ::Type{Array{T,N}}) where {T,N} = Array{rcum_promote_type(op,T), N} +rcum_convert(::Type{T}, x) where {T} = convert(T,x) +rcum_convert(::Type{T}, c::Char) where {T <: AbstractString} = T(string(c)) # accumulate_pairwise slightly slower then accumulate, but more numerically # stable in certain situations (e.g. sums). # it does double the number of operations compared to accumulate, @@ -977,7 +978,7 @@ function accumulate!(op, B, A, dim::Integer) # register usage and will be slightly faster ind1 = inds_t[1] @inbounds for I in CartesianIndices(tail(inds_t)) - tmp = convert(eltype(B), A[first(ind1), I]) + tmp = rcum_convert(eltype(B), A[first(ind1), I]) B[first(ind1), I] = tmp for i_1 = first(ind1)+1:last(ind1) tmp = op(tmp, A[i_1, I]) @@ -1027,7 +1028,7 @@ end # Copy the initial element in each 1d vector along dimension `dim` ii = first(ind) @inbounds for J in R2, I in R1 - B[I, ii, J] = A[I, ii, J] + B[I, ii, J] = rcum_convert(eltype(B), A[I, ii, J]) end # Accumulate @inbounds for J in R2, i in first(ind)+1:last(ind), I in R1 @@ -1075,7 +1076,7 @@ function _accumulate1!(op, B, v1, A::AbstractVector, dim::Integer) inds == linearindices(B) || throw(DimensionMismatch("linearindices of A and B don't match")) dim > 1 && return copyto!(B, A) i1 = inds[1] - cur_val = v1 + cur_val = rcum_convert(eltype(B), v1) B[i1] = cur_val @inbounds for i in inds[2:end] cur_val = op(cur_val, A[i]) diff --git a/test/arrayops.jl b/test/arrayops.jl index 644418579fb3b..c799ec575579b 100644 --- a/test/arrayops.jl +++ b/test/arrayops.jl @@ -2155,6 +2155,10 @@ end #25506 @test accumulate((acc, x) -> acc+x[1], 0, [(1,2), (3,4), (5,6)]) == [1, 4, 9] + @test accumulate(*, ['a', 'b']) == ["a", "ab"] + @inferred accumulate(*, String[]) + @test accumulate(*, ['a' 'b'; 'c' 'd'], 1) == ["a" "b"; "ac" "bd"] + @test accumulate(*, ['a' 'b'; 'c' 'd'], 2) == ["a" "ab"; "c" "cd"] end struct F21666{T <: Base.ArithmeticStyle} From 696c9161f96c564b94c151c4f3dd2690fea1cd65 Mon Sep 17 00:00:00 2001 From: Jan Weidner Date: Wed, 17 Jan 2018 09:01:59 +0100 Subject: [PATCH 3/5] get rid of rcum_convert --- base/multidimensional.jl | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/base/multidimensional.jl b/base/multidimensional.jl index 75c3d9e21336f..7e7a591769006 100644 --- a/base/multidimensional.jl +++ b/base/multidimensional.jl @@ -696,8 +696,6 @@ rcum_promote_type(op, ::Type{T}) where {T} = rcum_promote_type(op, T,T) # any AbstractArray here, but it's not clear how that would be possible rcum_promote_type(op, ::Type{Array{T,N}}) where {T,N} = Array{rcum_promote_type(op,T), N} -rcum_convert(::Type{T}, x) where {T} = convert(T,x) -rcum_convert(::Type{T}, c::Char) where {T <: AbstractString} = T(string(c)) # accumulate_pairwise slightly slower then accumulate, but more numerically # stable in certain situations (e.g. sums). # it does double the number of operations compared to accumulate, @@ -724,7 +722,7 @@ function accumulate_pairwise!(op::Op, result::AbstractVector, v::AbstractVector) n = length(li) n == 0 && return result i1 = first(li) - @inbounds result[i1] = v1 = v[i1] + @inbounds result[i1] = v1 = reduce_first(op,v[i1]) n == 1 && return result _accumulate_pairwise!(op, result, v, v1, i1+1, n-1) return result @@ -978,7 +976,7 @@ function accumulate!(op, B, A, dim::Integer) # register usage and will be slightly faster ind1 = inds_t[1] @inbounds for I in CartesianIndices(tail(inds_t)) - tmp = rcum_convert(eltype(B), A[first(ind1), I]) + tmp = reduce_first(op, A[first(ind1), I]) B[first(ind1), I] = tmp for i_1 = first(ind1)+1:last(ind1) tmp = op(tmp, A[i_1, I]) @@ -1028,7 +1026,7 @@ end # Copy the initial element in each 1d vector along dimension `dim` ii = first(ind) @inbounds for J in R2, I in R1 - B[I, ii, J] = rcum_convert(eltype(B), A[I, ii, J]) + B[I, ii, J] = reduce_first(op, A[I, ii, J]) end # Accumulate @inbounds for J in R2, i in first(ind)+1:last(ind), I in R1 @@ -1076,7 +1074,7 @@ function _accumulate1!(op, B, v1, A::AbstractVector, dim::Integer) inds == linearindices(B) || throw(DimensionMismatch("linearindices of A and B don't match")) dim > 1 && return copyto!(B, A) i1 = inds[1] - cur_val = rcum_convert(eltype(B), v1) + cur_val = reduce_first(op, v1) B[i1] = cur_val @inbounds for i in inds[2:end] cur_val = op(cur_val, A[i]) From 6069c7669f7fb35e7051cac18e5bc7a7a9c90882 Mon Sep 17 00:00:00 2001 From: Jan Weidner Date: Wed, 17 Jan 2018 13:42:20 +0100 Subject: [PATCH 4/5] get rid of obsolete rcum_promote_type --- base/multidimensional.jl | 18 ++++-------------- 1 file changed, 4 insertions(+), 14 deletions(-) diff --git a/base/multidimensional.jl b/base/multidimensional.jl index 7e7a591769006..ef0451b2aa352 100644 --- a/base/multidimensional.jl +++ b/base/multidimensional.jl @@ -685,16 +685,6 @@ _iterable(v) = Iterators.repeated(v) end end -## - -# see discussion in #18364 ... we try not to widen type of the resulting array -# from cumsum or cumprod, but in some cases (+, Bool) we may not have a choice. -rcum_promote_type(op, ::Type{T}, ::Type{S}) where {T,S} = promote_op(op, T, S) -rcum_promote_type(op, ::Type{T}) where {T} = rcum_promote_type(op, T,T) - -# handle sums of Vector{Bool} and similar. it would be nice to handle -# any AbstractArray here, but it's not clear how that would be possible -rcum_promote_type(op, ::Type{Array{T,N}}) where {T,N} = Array{rcum_promote_type(op,T), N} # accumulate_pairwise slightly slower then accumulate, but more numerically # stable in certain situations (e.g. sums). @@ -729,7 +719,7 @@ function accumulate_pairwise!(op::Op, result::AbstractVector, v::AbstractVector) end function accumulate_pairwise(op, v::AbstractVector{T}) where T - out = similar(v, rcum_promote_type(op, T)) + out = similar(v, promote_op(op, T)) return accumulate_pairwise!(op, out, v) end @@ -774,7 +764,7 @@ julia> cumsum(a,2) ``` """ function cumsum(A::AbstractArray{T}, dim::Integer) where T - out = similar(A, rcum_promote_type(+, T)) + out = similar(A, promote_op(+, T)) cumsum!(out, A, dim) end @@ -908,7 +898,7 @@ julia> accumulate(+, fill(1, 3, 3), 2) ``` """ function accumulate(op, A, dim::Integer) - out = similar(A, rcum_promote_type(op, eltype(A))) + out = similar(A, promote_op(op, eltype(A))) accumulate!(op, out, A, dim) end @@ -1057,7 +1047,7 @@ julia> accumulate(min, 0, [1,2,-1]) ``` """ function accumulate(op, v0, x::AbstractVector) - T = rcum_promote_type(op, typeof(v0), eltype(x)) + T = promote_op(op, typeof(v0), eltype(x)) out = similar(x, T) accumulate!(op, out, v0, x) end From 1b4472714e024b1ed7c807facc6d523022e6ea04 Mon Sep 17 00:00:00 2001 From: Jan Weidner Date: Wed, 17 Jan 2018 20:07:50 +0100 Subject: [PATCH 5/5] use two argument for of promote_op --- base/multidimensional.jl | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/base/multidimensional.jl b/base/multidimensional.jl index ef0451b2aa352..d476d53047f68 100644 --- a/base/multidimensional.jl +++ b/base/multidimensional.jl @@ -719,7 +719,7 @@ function accumulate_pairwise!(op::Op, result::AbstractVector, v::AbstractVector) end function accumulate_pairwise(op, v::AbstractVector{T}) where T - out = similar(v, promote_op(op, T)) + out = similar(v, promote_op(op, T, T)) return accumulate_pairwise!(op, out, v) end @@ -764,7 +764,7 @@ julia> cumsum(a,2) ``` """ function cumsum(A::AbstractArray{T}, dim::Integer) where T - out = similar(A, promote_op(+, T)) + out = similar(A, promote_op(+, T, T)) cumsum!(out, A, dim) end @@ -898,7 +898,7 @@ julia> accumulate(+, fill(1, 3, 3), 2) ``` """ function accumulate(op, A, dim::Integer) - out = similar(A, promote_op(op, eltype(A))) + out = similar(A, promote_op(op, eltype(A), eltype(A))) accumulate!(op, out, A, dim) end