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 preserve the AxisArray wrapper (fixes #55) #56

Merged
merged 3 commits into from
Mar 25, 2017

Conversation

timholy
Copy link
Member

@timholy timholy commented Feb 5, 2017

Very curious result if you say @code_warntype AxisArrays.reduced_indices(axes(A), (1,)). That's why some of the @inferred are commented out.

But otherwise this seems to work well.

Oh, argh! I'm running release-0.5 and this tests on 0.5.0. This is a problem...

@timholy
Copy link
Member Author

timholy commented Feb 5, 2017

CC @tknopp.

@tknopp
Copy link

tknopp commented Feb 5, 2017

not sure if I am actually able reviewing this but my understanding is that you hook into special methods (i.e. reduced_indices) that are used in base for reductions?!

The tests look good!

@timholy
Copy link
Member Author

timholy commented Feb 5, 2017

4f1da91 may be a deal-breaker.

@timholy
Copy link
Member Author

timholy commented Feb 21, 2017

Small fix made, rebased, and inference issue filed: JuliaLang/julia#20714. I use --inline=no so frequently in debugging that I think we can't merge this PR until this issue is fixed. Since it affects julia-0.5 as well as julia-0.6, I fear it could be a long wait.

@tknopp
Copy link

tknopp commented Mar 9, 2017

@timholy: This comes up very frequently and I still do not understand what the correct workaround is. Even the type unstable form maximum(A,1) does not work anymore.

So the question is: How do I get the maximum intensity projection of a 3D AxisArray and I want the result to be still an AxisArray

@timholy
Copy link
Member Author

timholy commented Mar 9, 2017

Try this:

function fixaxes(A, axs)
    ls = map(length, indices(A))
    AxisArray(A, map((i,l)->length(i) == l ? i : i(Base.OneTo(1)), axs, ls))
end

julia> A = AxisArray(collect(reshape(1:15,3,5)), :y, :x)
2-dimensional AxisArray{Int64,2,...} with axes:
    :y, Base.OneTo(3)
    :x, Base.OneTo(5)
And data, a 3×5 Array{Int64,2}:
 1  4  7  10  13
 2  5  8  11  14
 3  6  9  12  15

julia> fixaxes(maximum(parent(A), 1), axes(A))
2-dimensional AxisArray{Int64,2,...} with axes:                                                                                                                                              
    :y, Base.OneTo(1)                                                                                                                                                                        
    :x, Base.OneTo(5)                                                                                                                                                                        
And data, a 1×5 Array{Int64,2}:                                                                                                                                                              
 3  6  9  12  15

The parent isn't strictly necessary, but if you don't want your fix to break when this gets merged you're better off including it.

@tknopp
Copy link

tknopp commented Mar 9, 2017

My issue is more fundamental

julia> typeof(B)
AxisArrays.AxisArray{Float32,4,Array{Float32,4},Tuple{AxisArrays.Axis{:x,FloatRange{Float64}},AxisArrays.Axis{:y,FloatRange{Float64}},AxisArrays.Axis{:z,FloatRange{Float64}},AxisArrays.Axis{:time,FloatRange{Float64}}}}

julia> maximum(B,1)
ERROR: BoundsError: attempt to access (:x,:y,:z,:time)
  at index [5]
 in getindex(::Tuple{Symbol,Symbol,Symbol,Symbol}, ::Int64) at ./tuple.jl:8
 in reaxis(...) at /home/knopp/.julia/v0.5/AxisArrays/src/indexing.jl:42
 in macro expansion at ./reducedim.jl:222 [inlined]
 in macro expansion at ./simdloop.jl:73 [inlined]
 in _mapreducedim!(::Base.#identity, ::Base.#scalarmax, ::Array{Float32,4}, ::AxisArrays.AxisArray{Float32,4,Array{Float32,4},Tuple{AxisArrays.Axis{:x,FloatRange{Float64}},AxisArrays.Axis{:y,FloatRange{Float64}},AxisArrays.Axis{:z,FloatRange{Float64}},AxisArrays.Axis{:time,FloatRange{Float64}}}}) at ./reducedim.jl:221
 in mapreducedim!(::Function, ::Function, ::Array{Float32,4}, ::AxisArrays.AxisArray{Float32,4,Array{Float32,4},Tuple{AxisArrays.Axis{:x,FloatRange{Float64}},AxisArrays.Axis{:y,FloatRange{Float64}},AxisArrays.Axis{:z,FloatRange{Float64}},AxisArrays.Axis{:time,FloatRange{Float64}}}}) at ./reducedim.jl:237
 in mapreducedim(::Function, ::Function, ::AxisArrays.AxisArray{Float32,4,Array{Float32,4},Tuple{AxisArrays.Axis{:x,FloatRange{Float64}},AxisArrays.Axis{:y,FloatRange{Float64}},AxisArrays.Axis{:z,FloatRange{Float64}},AxisArrays.Axis{:time,FloatRange{Float64}}}}, ::Int64) at ./reducedim.jl:268
 in maximum(::AxisArrays.AxisArray{Float32,4,Array{Float32,4},Tuple{AxisArrays.Axis{:x,FloatRange{Float64}},AxisArrays.Axis{:y,FloatRange{Float64}},AxisArrays.Axis{:z,FloatRange{Float64}},AxisArrays.Axis{:time,FloatRange{Float64}}}}, ::Int64) at ./reducedim.jl:320

@timholy
Copy link
Member Author

timholy commented Mar 9, 2017

If you do that to parent(B) you won't have that problem; my suggested approach should work around that dandily.

That said, I didn't realize that was the problem. Maybe the same as #62? I'd forgotten about that one.

@tknopp
Copy link

tknopp commented Mar 10, 2017

Sorry for mixing things. As you said its two independent issues. But from a higher level its simply a regression on the previous Images.jl workflow. The parent thing does not work. But I open a dedicated issue with testcase.

@timholy timholy merged commit 8f8272d into master Mar 25, 2017
@timholy timholy deleted the teh/reductions branch March 25, 2017 13:18
@tkelman
Copy link

tkelman commented Mar 25, 2017

If this is something that differs between 0.5.0 and 0.5.1, is it worth testing 0.5.0 as a separate matrix entry on travis, given not everyone has upgraded right away to 0.5.1?

@timholy
Copy link
Member Author

timholy commented Mar 25, 2017

I didn't realize you do could that. See https://travis-ci.org/JuliaArrays/AxisArrays.jl/builds/215065371. I think I worked around the problem in a later commit, so I edited the comment above.

(Given that it's passing on 0.5.0, I don't think it's worth adding to the test matrix, but others might feel differently.)

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.

3 participants