Add `CategoricalVector` and `collapse` #88

iamed2 · 2017-06-05T20:05:46Z

CategoricalVector allows any type of vector to be a Categorical axis.

~~flatten~~ collapse concatenates AxisArrays with equal leading axes into a single AxisArray. All additional axes in any of the arrays are flattened into a single additional
CategoricalVector{Tuple} axis

I also changed axistrait to operate on types, with a fallback that calls typeof, in order to use it in @generated functions.

iamed2 · 2017-06-05T22:53:51Z

Nightly failure is a Julia bug (~~that I'm still trying to track down~~ EDIT: JuliaLang/julia#22248). The issue occurs with the loop at the end of test/core.jl. If I assign the loop variables to variables, the loop completes. If not, the loop hangs and Julia is uninterruptible. I will just assign the loop variables for now.

iamed2 · 2017-06-15T15:29:32Z

Julia bug is fixed now anyway.

timholy

This is excellent work. I have only one substantial change to suggest, but it's nontrivial. Don't hesitate to ask for help if my comments were confusing.

I should also add that I've not really used the categorical capabilities of AxisArrays, so it would be great if a second set of eyeballs could take a peek here.

But this is really nice, thanks (preliminarily) for the contribution!

timholy · 2017-06-22T00:28:06Z

test/categoricalvector.jl

+v = collect(zip([:a, :b, :c][rand(1:3,20)], [:x,:y][rand(1:2,20)], [:x,:y][rand(1:2,20)]))
+idx = sortperm(v)
+A = AxisArray(data[idx,:], CategoricalVector(v[idx]), [:a, :b])
+@test A[:b, :] == A[5:12, :]


I assume these reflect the actual random numbers produced due to srand(1234)? Would it be clearer to just hardcode v?

It would; I was just following test/sortedvector.jl as much as possible. I can change it though :)

timholy · 2017-06-23T00:19:17Z

REQUIRE

@@ -1,4 +1,5 @@
 julia 0.5
 IntervalSets
+Iterators


Let's use IterTools.jl instead, which was created to avoid the name conflicts that arose from having both Base.Iterators and the Iterators package.

Haha, I'm actually in the middle of making PRs to transition everyone from Iterators to IterTools! Only missed this because it's still in my 0.5 directory.

timholy · 2017-06-23T01:07:04Z

src/combine.jl

+    return _flatten(gca, As...; kwargs...)
+end
+
+function flatten(last_dim::Integer, As::AxisArray...; kwargs...)


The only sad thing here is that the result of flatten won't be inferrable. Perhaps you don't care, but if there could be an inferrable API, it might be nicer. If you're just comparing AxisArrays, you should be able to split the axes into "common" and "distinct" subsets. For example,

using AxisArrays # split out the first axis of each axisarray into a separate tuple @inline selectcommon(As::AxisArray...) = selectcommon(axes.(As)...) @inline selectcommon(axs::Tuple...) = _selectcommon(first.(axs), Base.tail.(axs)) # If the next axis has common name, keep it and then keep going @inline _selectcommon(axes1::NTuple{N, Axis{name}}, trailingaxes::NTuple{N, Any}) where {N,name} = (axes1, _selectcommon(first.(trailingaxes), Base.tail.(trailingaxes))...) # If they don't have the same Axis name, we drop the rest @inline _selectcommon(axes1, trailingaxes) = () A = AxisArray(rand(3,3,3), :x, :y, :z) B = AxisArray(rand(3,3,3), :x, :y, :t) cmn = selectcommon(A, B)

should generate a tuple-of-tuples, each element that containing one common axis from each input AxisArray (in this case, it generates (xtuple, ytuple) where xtuple contains the two :x axes and ytuple contains the two :y axes). You can get the (non-common) trailing axes with a call to Base.IteratorsMD.split, just using the N in the NTuple for the length of cmn.

I'll have to tweak this to add in the ability to limit the length of the common axes, maybe with Val{max_dim}

Oh no

JuliaLang/julia#22518

Not sure I fully understand the concern, can you point me to a use-case example?

If you really can't live without the arithmetic operations, then one can always define a MyVal struct here. There's nothing remotely special about Val per se, it's literally trivial. And there isn't any special support for it in inference.jl either:

tim@diva:~/src/julia-1.0/base$ grep -F "Val{" inference.jl tim@diva:~/src/julia-1.0/base$

@generated decrement(v::Type{Val{N}}) where N = :(Val{$(N-1)}) # split out the first axis of each axisarray into a separate tuple @inline selectcommon(v::Type{Val{D}}, As::AxisArray...) where D = selectcommon(v, axes.(As)...) @inline selectcommon(v::Type{Val{D}}, axs::Tuple...) where D = _selectcommon(v, first.(axs), Base.tail.(axs)) # If the next axis has common name, keep it and then keep going @inline _selectcommon(v::Type{Val{D}}, axes1::NTuple{N, Axis{name}}, trailingaxes::NTuple{N, Any}) where {D,N,name} = (axes1, _selectcommon(decrement(v), first.(trailingaxes), Base.tail.(trailingaxes))...) # If we have limited the axes to a certain number, stop @inline _selectcommon(::Type{Val{0}}, axes1::NTuple{N, Axis{name}}, trailingaxes::NTuple{N, Any}) where {N,name} = () # If they don't have the same Axis name, we drop the rest @inline _selectcommon(::Type{Val{D}}, axes1, trailingaxes) where D = ()

That seems like reasonable code, I just don't understand what you're trying to achieve. Under what circumstance would you flatten with fewer common axes than are available?

Ah okay, I'll try to explain our use case.

In an internal package we have an appendable feature set type which is index by some set of common indices. At any point we may want the "flattened" version. At any point we may push a new array to the set. We want the result of flattening a particular feature set to have the same axes always. We wouldn't want it to return different numbers of dimensions when there are only N arrays (where they share an extra axis) as when there are N+1 arrays (where array N+1 does not share the extra axis).

Does that make sense? Do you have alternate suggestions?

I realized that I really have two separate problems:

Flatten all axes after the common axes

Flatten all axes after axis N, ensuring axes 1:N are common

Trying to solve both in one function is creating strange mutant hybrid functions and causing increasing hacks. I think splitting these apart will be easier.

timholy · 2017-06-23T01:07:46Z

test/combine.jl

+A1 = AxisArray(A1data, Axis{:X}(1:2), Axis{:Y}(1:2))
+A2 = AxisArray(reshape(A2data, size(A2data)..., 1), Axis{:X}(1:2), Axis{:Y}(1:2), Axis{:Z}([:foo]))
+
+@test flatten(A1, A2; array_names=[:A1, :A2]) == AxisArray(cat(3, A1data, A2data), Axis{:X}(1:2), Axis{:Y}(1:2), Axis{:page}(CategoricalVector([(:A1,), (:A2, :foo)])))


If you do make an inferrable version of flatten, it would be great to add @inferred around those calls.

iamed2 · 2017-06-23T01:43:36Z

Thanks Tim! I'll work on getting the inferrable flatten tomorrow or Saturday.

iamed2 · 2017-06-23T02:56:26Z

Nightly failures are #92

iamed2 · 2017-07-06T16:55:58Z

I updated this to only include the fixed-dimension flatten. Hopefully this can get merged and I can do the select-flatten later.

This PR requires 0.6. I think it would be reasonable to have a separate PR to remove support for 0.5. What do you think?

timholy · 2017-07-08T22:10:25Z

Sorry I didn't notice this earlier.

I think it would be reasonable to have a separate PR to remove support for 0.5.

Either way would be fine, as long as it's a separate commit.

timholy · 2017-07-08T22:16:47Z

Seems fine to me. I'll let you decide how to handle the 0.6 issue and then I think this can be merged.

iamed2 · 2017-07-25T20:50:41Z

Can we merge #98 and this?

nalimilan · 2017-07-29T15:07:16Z

src/categoricalvector.jl

@@ -0,0 +1,85 @@
+
+export CategoricalVector


Please use a different name for this, as CategoricalVector is already used in CategoricalArrays, which replaced PooledDataArray in DataTables (and soon in DataFrames). Why not CategoricalAxis?

It's not an Axis, and it mirrors the SortedVector type.

Hmm, IIUC its only purpose is to treat it as a categorical axis, isn't it? Ideas about other possible names? It would be too bad to have conflicts when loading both AxisArrays and DataTables/DataFrames.

We could choose not to export it?

There aren't conflicts when you use both packages unless you also use CategoricalVector.

The nomenclature used within AxisArrays is Categorical, which is how CategoricalVector came up.

How about DiscreteVector?

nalimilan

I'm not very familiar with the AxisArray code, but flatten sounds a bit unexpected for this function. Base.Iterators.flatten does something very different. Also, this seems to mix two operations which are not necessarily related: concatenation and flattening. Shouldn't the latter be implemented as cat? Then flattening could be enabled as an option to cat.

nalimilan · 2017-07-29T15:13:42Z

src/combine.jl

+    flatten(::Type{Val{N}}, ::Type{NewArrayType}, As::AxisArray...) -> AxisArray
+    flatten(::Type{Val{N}}, ::Type{NewArrayType}, labels::Tuple, As::AxisArray...) -> AxisArray
+
+Concatenates AxisArrays with N equal leading axes into a single AxisArray.


Backquotes around type names (here and elsewhere).

nalimilan · 2017-07-29T15:14:25Z

src/combine.jl

+* `As::AxisArray...`: AxisArrays to be flattened together.
+"""
+@generated function flatten{N, AN, LType, NewArrayType<:AbstractArray}(
+    ::Type{Val{N}},


I don't think this is the conventional indentation.

There are only two existing cases of too-long method signatures and making this one look like those would mean pushing these arguments themselves too far to the right.

I'd do it anyway for consistency, and knowing that with the new where syntax the leading type parameters (which are the actual problem here) will be moved to the end of the line or to another line.

We can use the where syntax already (updated now)

nalimilan · 2017-07-29T15:18:39Z

src/combine.jl

+end
+
+"""
+    flatten(::Type{Val{N}}, As::AxisArray...) -> AxisArray


Could you add examples? I find it hard to understand what this function does.

Example which has been added:

julia> price_data = AxisArray(rand(10), Axis{:time}(Date(2016,01,01):Day(1):Date(2016,01,10))) 1-dimensional AxisArray{Float64,1,...} with axes: :time, 2016-01-01:1 day:2016-01-10 And data, a 10-element Array{Float64,1}: 0.885014 0.418562 0.609344 0.72221 0.43656 0.840304 0.455337 0.65954 0.393801 0.260207 julia> size_data = AxisArray(rand(10,2), Axis{:time}(Date(2016,01,01):Day(1):Date(2016,01,10)), Axis{:measure}([:area, :volume])) 2-dimensional AxisArray{Float64,2,...} with axes: :time, 2016-01-01:1 day:2016-01-10 :measure, Symbol[:area, :volume] And data, a 10×2 Array{Float64,2}: 0.159434 0.456992 0.344521 0.374623 0.522077 0.313256 0.994697 0.320953 0.95104 0.900526 0.921854 0.729311 0.000922581 0.148822 0.449128 0.761714 0.650277 0.135061 0.688773 0.513845 julia> flattened = flatten(Val{1}, (:price, :size), price_data, size_data) 2-dimensional AxisArray{Float64,2,...} with axes: :time, 2016-01-01:1 day:2016-01-10 :flat, Tuple{Symbol,Vararg{Symbol,N} where N}[(:price,), (:size, :area), (:size, :volume)] And data, a 10×3 Array{Float64,2}: 0.885014 0.159434 0.456992 0.418562 0.344521 0.374623 0.609344 0.522077 0.313256 0.72221 0.994697 0.320953 0.43656 0.95104 0.900526 0.840304 0.921854 0.729311 0.455337 0.000922581 0.148822 0.65954 0.449128 0.761714 0.393801 0.650277 0.135061 0.260207 0.688773 0.513845 julia> flattened[Axis{:flat}(:size)] == size_data true

iamed2 · 2017-07-31T17:35:57Z

I'm not very familiar with the AxisArray code, but flatten sounds a bit unexpected for this function. Base.Iterators.flatten does something very different. Also, this seems to mix two operations which are not necessarily related: concatenation and flattening. Shouldn't the latter be implemented as cat? Then flattening could be enabled as an option to cat.

The difference from cat is what happens to the axes when merged.

Also, something I want to do but have not yet done is allow merging of the leading axes, similar to join. In that case, it would be even more unlike cat.

nalimilan · 2017-07-31T21:47:22Z

src/categoricalvector.jl

+export CategoricalVector
+
+"""
+A CategoricalVector is an AbstractVector which is treated as a categorical axis regardless


Missing backticks around types in this docstring.

…ages

timholy

LGTM. A couple of tiny changes, and then someone should hit merge.

timholy · 2017-08-03T10:06:48Z

src/combine.jl

+    flat_axis_eltype = _flat_axis_eltype(LType, trailing_axes)
+    flat_axis_type = CategoricalVector{flat_axis_eltype, Vector{flat_axis_eltype}}
+
+    new_axes_type = Tuple{new_common_axes..., Axis{:flat, flat_axis_type}}


:collapsed?

timholy · 2017-08-03T10:06:59Z

src/combine.jl

+end
+
+@generated function collapse{N, AN, LType}(::Type{Val{N}}, labels::NTuple{AN, LType}, As::Vararg{AxisArray, AN})
+    flat_dim = Val{N + 1}


timholy · 2017-08-03T10:08:40Z

src/combine.jl

+    collapse(::Type{Val{N}}, ::Type{NewArrayType}, labels::Tuple, As::AxisArray...) -> AxisArray
+
+Collapses `AxisArray`s with `N` equal leading axes into a single `AxisArray`.
+All additional axes in any of the arrays are flattened into a single additional


timholy · 2017-08-03T10:09:29Z

src/combine.jl

+
+Collapses `AxisArray`s with `N` equal leading axes into a single `AxisArray`.
+All additional axes in any of the arrays are flattened into a single additional
+`CategoricalVector{Tuple}` axis.


Comment on the label of the new axis?

timholy · 2017-08-03T10:18:04Z

src/combine.jl

+                    arrays. The remaining axes are flattened. All `N` axes must be common
+                    to each input array, at the same dimension. Values from `0` up to the
+                    minimum number of dimensions across all input arrays are allowed.
+* `labels::Tuple`: (optional) a label for each `AxisArray` in `As` which is used in the flat


Not entirely clear. Maybe "Index assigned to each array in As in the :collapsed axis"? Since it's optional, also specify what happens by default.

timholy · 2017-08-03T10:19:03Z

src/combine.jl

+julia> collapsed = collapse(Val{1}, (:price, :size), price_data, size_data)
+2-dimensional AxisArray{Float64,2,...} with axes:
+    :time, 2016-01-01:1 day:2016-01-10
+    :flat, Tuple{Symbol,Vararg{Symbol,N} where N}[(:price,), (:size, :area), (:size, :volume)]


This will need to be fixed if you change the name to :collapsed

timholy · 2017-08-03T19:03:48Z

Thanks for your patience, and of course for the contribution!

iamed2 · 2017-08-03T19:33:47Z

Thank you for the attention!! :D

iamed2 force-pushed the flatten branch from 801b8d0 to 428124a Compare June 5, 2017 21:16

iamed2 mentioned this pull request Jun 15, 2017

RFC: Re-enable axis checks in constructor #91

Open

timholy reviewed Jun 23, 2017

View reviewed changes

iamed2 added 6 commits July 7, 2017 11:04

Make axistrait operate on types

6ca84d4

Add CategoricalVector type

46982e2

Add flatten

0bc92de

Fix mysterious 0.7 issue

85eda7c

Switch from Iterators.jl to IterTools.jl

5958549

Reduce to flatten{N} only

3c29df0

iamed2 force-pushed the flatten branch from 111f0d0 to 3c29df0 Compare July 7, 2017 16:05

iamed2 mentioned this pull request Jul 8, 2017

Make axistrait operate on types #95

Merged

timholy approved these changes Jul 8, 2017

View reviewed changes

iamed2 mentioned this pull request Jul 20, 2017

Remove support for 0.5 and drop Compat #98

Merged

Merge branch 'master' into flatten

b39560f

iamed2 added 2 commits July 26, 2017 15:35

Merge branch 'master' into flatten

6ac9f9f

Allow passing in output array data type to flatten

eb1a15d

nalimilan reviewed Jul 29, 2017

View reviewed changes

Add example to flatten

e96a36a

iamed2 added 2 commits July 31, 2017 16:04

Adjust flatten indentation

4f4292a

Rename flatten to collapse

e2796cb

nalimilan reviewed Jul 31, 2017

View reviewed changes

Remove CategoricalVector export due to name conflicts with other pack…

42f5483

…ages

iamed2 changed the title ~~Add CategoricalVector and flatten~~ Add CategoricalVector and collapse Jul 31, 2017

timholy reviewed Aug 3, 2017

View reviewed changes

Change all flat-related terminology to collapse

6575d4b

timholy merged commit f0743ce into JuliaArrays:master Aug 3, 2017

omus deleted the flatten branch December 28, 2017 21:49

Add CategoricalVector and collapse #88

Add CategoricalVector and collapse #88

Conversation

iamed2 commented Jun 5, 2017 • edited Loading

iamed2 commented Jun 5, 2017 • edited Loading

iamed2 commented Jun 15, 2017

timholy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iamed2 commented Jun 23, 2017

iamed2 commented Jun 23, 2017

iamed2 commented Jul 6, 2017

timholy commented Jul 8, 2017

timholy commented Jul 8, 2017

iamed2 commented Jul 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nalimilan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iamed2 Jul 31, 2017 • edited Loading

Choose a reason for hiding this comment

iamed2 commented Jul 31, 2017

Choose a reason for hiding this comment

timholy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timholy commented Aug 3, 2017

iamed2 commented Aug 3, 2017

Add `CategoricalVector` and `collapse` #88

Add `CategoricalVector` and `collapse` #88

iamed2 commented Jun 5, 2017 •

edited

Loading

iamed2 commented Jun 5, 2017 •

edited

Loading

iamed2 Jul 31, 2017 •

edited

Loading