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.

Sign up for GitHub

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

Jump to bottom

Add test cases for AxisArrays #30

Closed
rofinn opened this issue Jul 16, 2019 · 7 comments · Fixed by #39
Closed

Add test cases for AxisArrays #30

rofinn opened this issue Jul 16, 2019 · 7 comments · Fixed by #39
Assignees
@rofinn
Milestone
1.0

Comments

@rofinn
Copy link
Member

rofinn commented Jul 16, 2019

We should probably confirm that all our imputation methods work on AxisArrays as well
@rofinn rofinn self-assigned this Jul 16, 2019
@rofinn rofinn added this to the 1.0 milestone Jul 16, 2019
@nickrobinson251
Copy link
Contributor

Current situation (master @ adfef4dbcbe5f910610fc1a048efe1c623db38a6)

julia> using Impute, AxisArrays, Statistics
┌ Warning: ...<snip>
└ @ Impute ~/.julia/packages/Impute/OIgZp/src/Impute.jl:22

julia> A = AxisArray(rand(2, 3));

julia> impute!(A, Impute.Fill(; value=mean))
ERROR: MethodError: no method matching deleteat!(::AxisArray{Float64,1,SubArray{Float64,1,Array{Float64,2},Tuple{Base.Slice{Base.OneTo{Int64}},Int64},true},Tuple{Axis{:row,UnitRange{Int64}}}}, ::UnitRange{Int64})
Closest candidates are:
  deleteat!(::BitArray{1}, ::UnitRange{Int64}) at bitarray.jl:916
  deleteat!(::Array{T,1} where T, ::UnitRange{#s72} where #s72<:Integer) at array.jl:1178
  deleteat!(::Array{T,1} where T, ::AbstractArray{T,1} where T) at array.jl:1213
  ...
Stacktrace:
 [1] filter!(::getfield(Impute, Symbol("##15#17")){Impute.Context}, ::AxisArray{Float64,1,SubArray{Float64,1,Array{Float64,2},Tuple{Base.Slice{Base.OneTo{Int64}},Int64},true},Tuple{Axis{:row,UnitRange{Int64}}}}) at ./array.jl:2346
 [2] #14 at /Users/nick/.julia/packages/Impute/OIgZp/src/imputors/drop.jl:35 [inlined]
 [3] (::Impute.Context)(::getfield(Impute, Symbol("##14#16")){AxisArray{Float64,1,SubArray{Float64,1,Array{Float64,2},Tuple{Base.Slice{Base.OneTo{Int64}},Int64},true},Tuple{Axis{:row,UnitRange{Int64}}}}}) at /Users/nick/.julia/packages/Impute/OIgZp/src/context.jl:227
 [4] impute!(::AxisArray{Float64,1,SubArray{Float64,1,Array{Float64,2},Tuple{Base.Slice{Base.OneTo{Int64}},Int64},true},Tuple{Axis{:row,UnitRange{Int64}}}}, ::Impute.DropObs) at /Users/nick/.julia/packages/Impute/OIgZp/src/imputors/drop.jl:34
 [5] impute at /Users/nick/.julia/packages/Impute/OIgZp/src/imputors.jl:39 [inlined]
 [6] #drop#60(::Base.Iterators.Pairs{Symbol,Impute.Context,Tuple{Symbol},NamedTuple{(:context,),Tuple{Impute.Context}}}, ::Function, ::AxisArray{Float64,1,SubArray{Float64,1,Array{Float64,2},Tuple{Base.Slice{Base.OneTo{Int64}},Int64},true},Tuple{Axis{:row,UnitRange{Int64}}}}) at /Users/nick/.julia/packages/Impute/OIgZp/src/Impute.jl:64
 [7] #drop at ./none:0 [inlined]
 [8] (::getfield(Impute, Symbol("##47#48")){AxisArray{Float64,1,SubArray{Float64,1,Array{Float64,2},Tuple{Base.Slice{Base.OneTo{Int64}},Int64},true},Tuple{Axis{:row,UnitRange{Int64}}}},Fill{typeof(mean)}})(::Impute.Context) at /Users/nick/.julia/packages/Impute/OIgZp/src/imputors/fill.jl:42
 [9] (::Impute.Context)(::getfield(Impute, Symbol("##47#48")){AxisArray{Float64,1,SubArray{Float64,1,Array{Float64,2},Tuple{Base.Slice{Base.OneTo{Int64}},Int64},true},Tuple{Axis{:row,UnitRange{Int64}}}},Fill{typeof(mean)}}) at /Users/nick/.julia/packages/Impute/OIgZp/src/context.jl:227
 [10] impute!(::AxisArray{Float64,1,SubArray{Float64,1,Array{Float64,2},Tuple{Base.Slice{Base.OneTo{Int64}},Int64},true},Tuple{Axis{:row,UnitRange{Int64}}}}, ::Fill{typeof(mean)}) at /Users/nick/.julia/packages/Impute/OIgZp/src/imputors/fill.jl:39
 [11] impute!(::AxisArray{Float64,2,Array{Float64,2},Tuple{Axis{:row,Base.OneTo{Int64}},Axis{:col,Base.OneTo{Int64}}}}, ::Fill{typeof(mean)}) at /Users/nick/.julia/packages/Impute/OIgZp/src/imputors.jl:72
 [12] top-level scope at none:0

And attempt to add work-arounds... this one doesn't work (despite this SubArray work-around)

julia> Base.deleteat!(A::AxisArray, i) = deleteat!(parent(A), i)

julia> impute!(A, Impute.Fill(; value=mean))
ERROR: MethodError: no method matching deleteat!(::SubArray{Float64,1,Array{Float64,2},Tuple{Base.Slice{Base.OneTo{Int64}},Int64},true}, ::UnitRange{Int64})
Closest candidates are:
  deleteat!(::BitArray{1}, ::UnitRange{Int64}) at bitarray.jl:916
  deleteat!(::Array{T,1} where T, ::UnitRange{#s72} where #s72<:Integer) at array.jl:1178
  deleteat!(::Array{T,1} where T, ::AbstractArray{T,1} where T) at array.jl:1213
  ...
Stacktrace:
 [1] deleteat!(::AxisArray{Float64,1,SubArray{Float64,1,Array{Float64,2},Tuple{Base.Slice{Base.OneTo{Int64}},Int64},true},Tuple{Axis{:row,UnitRange{Int64}}}}, ::UnitRange{Int64}) at ./REPL[4]:1
 [2] filter!(::getfield(Impute, Symbol("##15#17")){Impute.Context}, ::AxisArray{Float64,1,SubArray{Float64,1,Array{Float64,2},Tuple{Base.Slice{Base.OneTo{Int64}},Int64},true},Tuple{Axis{:row,UnitRange{Int64}}}}) at ./array.jl:2346
 [3] #14 at /Users/nick/.julia/packages/Impute/OIgZp/src/imputors/drop.jl:35 [inlined]
 [4] (::Impute.Context)(::getfield(Impute, Symbol("##14#16")){AxisArray{Float64,1,SubArray{Float64,1,Array{Float64,2},Tuple{Base.Slice{Base.OneTo{Int64}},Int64},true},Tuple{Axis{:row,UnitRange{Int64}}}}}) at /Users/nick/.julia/packages/Impute/OIgZp/src/context.jl:227
 [5] impute!(::AxisArray{Float64,1,SubArray{Float64,1,Array{Float64,2},Tuple{Base.Slice{Base.OneTo{Int64}},Int64},true},Tuple{Axis{:row,UnitRange{Int64}}}}, ::Impute.DropObs) at /Users/nick/.julia/packages/Impute/OIgZp/src/imputors/drop.jl:34
 [6] impute at /Users/nick/.julia/packages/Impute/OIgZp/src/imputors.jl:39 [inlined]
 [7] #drop#60(::Base.Iterators.Pairs{Symbol,Impute.Context,Tuple{Symbol},NamedTuple{(:context,),Tuple{Impute.Context}}}, ::Function, ::AxisArray{Float64,1,SubArray{Float64,1,Array{Float64,2},Tuple{Base.Slice{Base.OneTo{Int64}},Int64},true},Tuple{Axis{:row,UnitRange{Int64}}}}) at /Users/nick/.julia/packages/Impute/OIgZp/src/Impute.jl:64
 [8] #drop at ./none:0 [inlined]
 [9] (::getfield(Impute, Symbol("##47#48")){AxisArray{Float64,1,SubArray{Float64,1,Array{Float64,2},Tuple{Base.Slice{Base.OneTo{Int64}},Int64},true},Tuple{Axis{:row,UnitRange{Int64}}}},Fill{typeof(mean)}})(::Impute.Context) at /Users/nick/.julia/packages/Impute/OIgZp/src/imputors/fill.jl:42
 [10] (::Impute.Context)(::getfield(Impute, Symbol("##47#48")){AxisArray{Float64,1,SubArray{Float64,1,Array{Float64,2},Tuple{Base.Slice{Base.OneTo{Int64}},Int64},true},Tuple{Axis{:row,UnitRange{Int64}}}},Fill{typeof(mean)}}) at /Users/nick/.julia/packages/Impute/OIgZp/src/context.jl:227
 [11] impute!(::AxisArray{Float64,1,SubArray{Float64,1,Array{Float64,2},Tuple{Base.Slice{Base.OneTo{Int64}},Int64},true},Tuple{Axis{:row,UnitRange{Int64}}}}, ::Fill{typeof(mean)}) at /Users/nick/.julia/packages/Impute/OIgZp/src/imputors/fill.jl:39
 [12] impute!(::AxisArray{Float64,2,Array{Float64,2},Tuple{Axis{:row,Base.OneTo{Int64}},Axis{:col,Base.OneTo{Int64}}}}, ::Fill{typeof(mean)}) at /Users/nick/.julia/packages/Impute/OIgZp/src/imputors.jl:72
 [13] top-level scope at none:0

This one works:

julia> Base.deleteat!(A::AxisArray, i) = deleteat!(collect(parent(A)), i)

julia> impute!(A, Impute.Fill(; value=mean))
2×3 AxisArray{Float64,2,Array{Float64,2},Tuple{Axis{:row,Base.OneTo{Int64}},Axis{:col,Base.OneTo{Int64}}}}:
 0.521519  0.0250059  0.804754
 0.139592  0.135141   0.00881104

Here's the set-up:

julia> VERSION
v"1.1.1"

(tmp.zMtraSEJ) pkg> status
    Status `/private/var/folders/hx/1h0bbkfd18d4n1qrnwmrl4j00000gn/T/tmp.zMtraSEJ/Project.toml`
  [39de3d68] AxisArrays v0.3.0
  [f7bf1975] Impute v0.2.0 #master (https://github.com/invenia/Impute.jl.git)

@rofinn
Copy link
Member Author

rofinn commented Jul 16, 2019 •
edited
Loading

It'd be nice if we could have some kind of AbstractSubArray type, so the SubArray fix could work on both AxisArray{T, N, <:SubArray} and SubArray by being a subtype of that abstract type. This is the ongoing issue I have with array wrapper types like axisarrays.

@nickrobinson251
Copy link
Contributor

workaround is impute!(parent(A), args...)

@rofinn rofinn mentioned this issue Jul 31, 2019
Merged
@rofinn
Copy link
Member Author

rofinn commented Jul 31, 2019

Alright, the above PR should make that the default behaviour. The only part that I don't like about this is approach is that we're changing the return type for dropobs because setparent! isn't a thing.

@nickrobinson251
Copy link
Contributor

Would you mind running a case /showing the behaviour with another wrapper-array type, like NamedDimsArray?

@rofinn
Copy link
Member Author

rofinn commented Jul 31, 2019 •
edited
Loading 

julia> nda = NamedDimsArray{(:x, :y)}(m)
5×4 NamedDimsArray{(:x, :y),Union{Missing, Float64},2,Array{Union{Missing, Float64},2}}:
 1.0        6.0       11.0  16.0
  missing    missing  12.0  17.0
  missing   8.0       13.0  18.0
 4.0        9.0       14.0  19.0
 5.0       10.0       15.0  20.0

julia> Impute.fill(nda)
5×4 NamedDimsArray{(:x, :y),Union{Missing, Float64},2,Array{Union{Missing, Float64},2}}:
  1.0      6.0     11.0  16.0
 10.8333  11.2396  12.0  17.0
 10.8333   8.0     13.0  18.0
  4.0      9.0     14.0  19.0
  5.0     10.0     15.0  20.0

julia> Impute.dropobs(nda)
3×4 NamedDimsArray{(:x, :y),Union{Missing, Float64},2,Array{Union{Missing, Float64},2}}:
 1.0   6.0  11.0  16.0
 4.0   9.0  14.0  19.0
 5.0  10.0  15.0  20.0

julia> Impute.dropvars(nda)
5×2 NamedDimsArray{(:x, :y),Union{Missing, Float64},2,Array{Union{Missing, Float64},2}}:
 11.0  16.0
 12.0  17.0
 13.0  18.0
 14.0  19.0
 15.0  20.0

@rofinn
Copy link
Member Author

rofinn commented Jul 31, 2019

Oops, n/m I was on the wrong branch.

@rofinn rofinn closed this as completed in #39 Aug 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@rofinn rofinn

Labels
None yet
Projects
None yet
Milestone
1.0
Development

Successfully merging a pull request may close this issue.

Add more tests invenia/Impute.jl
2 participants
@rofinn @nickrobinson251