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

Reductions fail with Unitful axes #113

Open
ajkeller34 opened this issue Aug 22, 2017 · 1 comment
Open

Reductions fail with Unitful axes #113

ajkeller34 opened this issue Aug 22, 2017 · 1 comment

Comments

@ajkeller34
Copy link
Collaborator

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 #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
Copy link
Collaborator

iamed2 commented Aug 22, 2017

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

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

No branches or pull requests

2 participants