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

Constructor over an AxisArray #105

Closed mdavezac closed 7 years ago

mdavezac commented 7 years ago

Implements and closes #103

mdavezac commented 7 years ago

Currently, the implementation is not type-stable, probably because of the tuple slicing.

I can see how to implement a types-stable constructor by making the constructor @generated and manufacturing the tuple as code, but is there a more direct way?

iamed2 commented 7 years ago

To address the tuple slicing, you can pull the length of the provided axes out as a type parameter N (Vararg{Axis, N}) and then use last(Base.IteratorsMD.split(axes(A), Val{N})) to get the trailing axes.

mdavezac commented 7 years ago

Using @iamed2's trickery, the constructor is now type-stable.

I had to define both AxisArray(A::AxisArray, ax::Tuple{Axis, Vararg{Axis, N}) where N and AxisArray(A::AxisArray, ax::Vararg{Axis, N}) where N to achieve type stability in all cases. Nominally, only the former is necessary since a chain of constructors already exist that provides the functionality of the latter. However, without the explicit shortcut, type-stability fails in some cases (e.g in the current tests).

So this ready to merge, I think

mdavezac commented 7 years ago

For the record, without the Vararg constructor, the following fails:

using AxisArrays
A = AxisArray(reshape(1:24, 2,3,4), Axis{:x}(1:2), Axis{:y}(1:3), Axis{:z}(1:4))
Base.Test.@inferred AxisArray(A, Axis{:yoyo}(1:2))
Base.Test.@inferred AxisArray(A, (Axis{:yoyo}(1:2),))

And removing the third would make it work.

iamed2 commented 7 years ago

Does it have to be (::Type{AxisArray}) instead of AxisArray? If so, why?

mdavezac commented 7 years ago

@iamed2, modified to AxisArray

iamed2 commented 7 years ago

Well I think this is ready :) just needs someone to merge

ajkeller34 commented 7 years ago

Looks great. Thank you @mdavezac! I just made two minor comments regarding the tests but I'll merge after those are addressed.

mdavezac commented 7 years ago

Thanks @ajkeller34, I've modified the tests based on your comments