search

JuliaArrays / AxisArrays.jl

Performant arrays where each dimension can have a named axis with values
http://JuliaArrays.github.io/AxisArrays.jl/latest/
Other
200 stars 42 forks source link

Reductions fail with Unitful axes #113

Open ajkeller34 opened 7 years ago

ajkeller34 commented 7 years ago 
julia> using Unitful, AxisArrays

julia> C = AxisArray(collect(reshape(1:15,3,5)), Axis{:y}([2.0,3.0,4.5]u"m"), Axis{:x}([2.0,2.1,2.3,2.5,2.6]u"cm"))
2-dimensional AxisArray{Int64,2,...} with axes:
    :y, Quantity{Float64, Dimensions:{𝐋}, Units:{m}}[2.0 m, 3.0 m, 4.5 m]
    :x, Quantity{Float64, Dimensions:{𝐋}, Units:{cm}}[2.0 cm, 2.1 cm, 2.3 cm, 2.5 cm, 2.6 cm]
And data, a 3Γ—5 Array{Int64,2}:
 1  4  7  10  13
 2  5  8  11  14
 3  6  9  12  15

julia> sum(C, 1)
ERROR: DimensionError: m and 1 are not dimensionally compatible.
Stacktrace:
 [1] copy!(::IndexLinear, ::Array{Quantity{Float64, Dimensions:{𝐋}, Units:{m}},1}, ::IndexLinear, ::Base.OneTo{Int64}) at ./abstractarray.jl:655
 [2] reduced_axis at /Users/ajkeller/.julia/v0.6/AxisArrays/src/core.jl:345 [inlined]
 [3] (::AxisArrays.##5#6{Tuple{Int64}})(::AxisArrays.Axis{:y,Array{Quantity{Float64, Dimensions:{𝐋}, Units:{m}},1}}, ::Int64) at /Users/ajkeller/.julia/v0.6/AxisArrays/src/core.jl:306
 [4] mapreducedim at ./reducedim.jl:241 [inlined]
 [5] sum at ./reducedim.jl:570 [inlined]
 [6] sum(::AxisArrays.AxisArray{Int64,2,Array{Int64,2},Tuple{AxisArrays.Axis{:y,Array{Quantity{Float64, Dimensions:{𝐋}, Units:{m}},1}},AxisArrays.Axis{:x,Array{Quantity{Float64, Dimensions:{𝐋}, Units:{cm}},1}}}}, ::Int64) at ./reducedim.jl:572

Would be fixed by redefining reduced_axis and reduced_axis0 in src/core.jl:

reduced_axis( ax) = ax(Base.OneTo(1))
reduced_axis0(ax) = ax(length(ax.val) == 0 ? Base.OneTo(0) : Base.OneTo(1))

However, this change would be at the expense of inferrability in certain cases, resulting in some regressions.

I noticed this issue when looking over https://github.com/JuliaArrays/AxisArrays.jl/pull/112. That PR improves the situation for other types but does not fix this issue, because Unitful.Quantity <: Number. One possible fix based on that PR would be to have an inferrable reduced_axis for Union{Real,Complex} rather than Number but I'm not sure what the broader consequences would be. It seems like a lot of Number subtypes defined in packages are actually <: Real (e.g. Measurements.Measurement, ForwardDiff.Dual) so maybe this would be acceptable.

iamed2 commented 7 years ago

I might be able to tackle this using oneunit. I'll try to solve this tomorrow.