JuliaArrays / JuliaArrays/AxisArrays.jl

Reductions fail with Unitful axes

Open
#113 1 comment 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
Julia
Stars
207
Forks
41
PR merge metrics
No merged PRs in 30d

Description

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.

Contributor guide

No contributing guide indexed for this repository

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start in src/core.jl with reduced_axis and reduced_axis0, then reproduce the Unitful example using sum(C, 1). Review pull request 112 and compare the proposed handling for Number versus Real and Complex types; done means reductions work with Unitful axes without the noted inferability regressions.

Written by the indexing model from the issue text.

Assessment

Tech stack
julia
Domain
data
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
28/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.