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

Major refactor and update for 1.0 #5

Merged
merged 3 commits into from
Jan 24, 2019
Merged

Major refactor and update for 1.0 #5

merged 3 commits into from
Jan 24, 2019

Conversation

mbauman
Copy link
Collaborator

@mbauman mbauman commented Jan 23, 2019

Use a new lazy iterator forumalation to avoid excessive allocations (a la Base.LogicalIndex). Fixes #2 and should fix #1 (although I've not run any benchmarks yet).

greimel and others added 3 commits January 22, 2019 23:59
Use a new lazy iterator forumalation to avoid excessive allocations (a la Base.LogicalIndex). Fixes #2 and should fix #1 (although I've not run any benchmarks yet).
@codecov-io

This comment has been minimized.

@yurivish
Copy link

This is great!

Is there any way to make custom index packages like this one work with arrays that have custom indexing?

I've been playing around with my own custom array type trying to figure out how to make it work, but then noticed that AxisArrays also doesn't support them:

julia> using AxisArrays, InvertedIndices

julia> B = AxisArray(reshape(1:15, 5, 3), .1:.1:0.5, [:a, :b, :c]);

julia> B[:, :b]
5-element AxisArray{Int64,1,Array{Int64,1},Tuple{Axis{:row,StepRangeLen{Float64,Base.TwicePrecision{Float64},Base.TwicePrecision{Float64}}}}}:
  6
  7
  8
  9
 10

julia> B[:, Not(:b)]
ERROR: ArgumentError: index InvertedIndex{Symbol}(:b) not found
Stacktrace:
 [1] axisindexes at /Users/yurivish/.julia/packages/AxisArrays/G6pZY/src/indexing.jl:290 [inlined]
 [2] axisindexes at /Users/yurivish/.julia/packages/AxisArrays/G6pZY/src/indexing.jl:174 [inlined]
 [3] macro expansion at /Users/yurivish/.julia/packages/AxisArrays/G6pZY/src/indexing.jl:354 [inlined]
 [4] to_index(::AxisArray{Int64,2,Base.ReshapedArray{Int64,2,UnitRange{Int64},Tuple{}},Tuple{Axis{:row,StepRangeLen{Float64,Base.TwicePrecision{Float64},Base.TwicePrecision{Float64}}},Axis{:col,Array{Symbol,1}}}}, ::Colon, ::InvertedIndex{Symbol}) at /Users/yurivish/.julia/packages/AxisArrays/G6pZY/src/indexing.jl:311
 [5] getindex(::AxisArray{Int64,2,Base.ReshapedArray{Int64,2,UnitRange{Int64},Tuple{}},Tuple{Axis{:row,StepRangeLen{Float64,Base.TwicePrecision{Float64},Base.TwicePrecision{Float64}}},Axis{:col,Array{Symbol,1}}}}, ::Function, ::InvertedIndex{Symbol}) at /Users/yurivish/.julia/packages/AxisArrays/G6pZY/src/indexing.jl:117
 [6] top-level scope at none:0

NamedArrays implements its own Not here.

@mbauman
Copy link
Collaborator Author

mbauman commented Jan 23, 2019

If AxisArrays properly overloaded (and called) Base.to_indices instead of defining its crufty old to_index function it would just work™.

This PR asks to_indices to provide the proper index type for the skipped indices, so this will play nicely with any package that overloads that API. Then the indexing definitions in other packages will need to call to_indices themselves if they accept ::Any index types. That's the new hotness and how I'd recommend doing things these days. I explicitly designed Base.to_indices with the explicit intention of someday using it in AxisArrays... just hasn't happened yet.

@yurivish
Copy link

Awesome. Thank you!

@mbauman mbauman merged commit 1c14ef1 into master Jan 24, 2019
@mbauman mbauman deleted the mb/1.0 branch January 24, 2019 00:52
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.

Update to 1.0? Speed up opportunities?
4 participants