Repository: invenia/NamedDims.jl (julia)

Issue:
Indexing by BitArray{N}
This doesn’t error, but why should it have name `:x` not `:z`? Probably it should have neither (ref #69):
```
julia> nda = NamedDimsArray(rand(1:9, 3,3,3), (:x, :y, :z));

julia> nda[rand(3,3,3) .> 0.5]
17-element NamedDimsArray{(:x,),Int64,1,Array{Int64,1}}:
```
The case of `BitArray{1}` works correctly:
```
julia> nda[rand(3) .> 0.3, 1, :]
2×3 NamedDimsArray{(:x, :z),Int64,2,Array{Int64,2}}:
```

Pull request description:
Closes #70

Patch:
diff --git a/Project.toml b/Project.toml
index f81c8ef..9dc9559 100644
--- a/Project.toml
+++ b/Project.toml
@@ -1,7 +1,7 @@
 name = "NamedDims"
 uuid = "356022a1-0364-5f58-8944-0da4b18d706f"
 authors = ["Invenia Technical Computing Corporation"]
-version = "0.2.20"
+version = "0.2.21"
 
 [deps]
 LinearAlgebra = "37e2e46d-f89d-539d-b4ee-838fcccc9c8e"
diff --git a/src/name_core.jl b/src/name_core.jl
index 781919a..d3f44c0 100644
--- a/src/name_core.jl
+++ b/src/name_core.jl
@@ -268,12 +268,20 @@ is_noninteger_type(::Any) = true
 
 """
     remaining_dimnames_from_indexing(dimnames::Tuple, inds)
-Given a tuple of dimension names
-and a set of index expressesion e.g `1, :, 1:3, [true, false]`,
-determine which are not dropped.
-Dimensions indexed with scalars are dropped
+
+Given a tuple of dimension names,
+and a tuple of indices e.g `(1, :, 1:3, [true, false])`,
+this drops those indexed with scalars or `CartesianIndex`,
+inserts `:_` for `newaxis = [CartesianIndex{0}()]`,
+and returns another tuple of names.
+
+It will return an empty tuple to indicate that all names should be dropped.
+This happend for scalar indexing by integers, or one `CartesianIndex`.
+It also happens e.g. when indexing a matrix by a `BitArray{2}` such as `mat[mat .> 0.5]`:
+this returns a vector, the same as vec(mat)[vec(mat .> 0.5)], whose dimension isn't any
+of the original dimensions, hence has no name.
 """
-@generated function remaining_dimnames_from_indexing(dimnames::Tuple, inds)
+@generated function remaining_dimnames_from_indexing(dimnames::Tuple, inds::Tuple)
     # 0-Allocation see:
     # `@btime (()->remaining_dimnames_from_indexing((:a, :b, :c), (:,390,:)))()``
     keep_names = []
@@ -283,8 +291,13 @@ Dimensions indexed with scalars are dropped
             dim_num += 1
         elseif type <: CartesianIndex
             dim_num += type.parameters[1]
-        elseif type == Array{CartesianIndex{0},1}
+        elseif type <: AbstractVector{CartesianIndex{0}}
             push!(keep_names, QuoteNode(:_))
+        elseif type <: AbstractArray{<:Integer} && ndims(type) > 1
+            dim_num += 1
+            for _ in 1:ndims(type)
+                push!(keep_names, QuoteNode(:_))
+            end
         else
             dim_num += 1
             push!(keep_names, :(getfield(dimnames, $dim_num)) )
@@ -293,6 +306,15 @@ Dimensions indexed with scalars are dropped
     return Expr(:call, :compile_time_return_hack, Expr(:tuple, keep_names...))
 end
 
+remaining_dimnames_from_indexing(dn::Tuple, inds::Tuple{Vararg{<:Integer}}) = ()
+remaining_dimnames_from_indexing(dn::Tuple, ci::Tuple{CartesianIndex}) = ()
+
+function remaining_dimnames_from_indexing(
+    dimnames::Tuple{<:Any, <:Any, Vararg}, inds::Tuple{T}
+) where T <: Union{Base.LogicalIndex, AbstractVector{<:CartesianIndex}}
+    return ()
+end
+
 """
     remaining_dimnames_after_dropping(dimnames::Tuple, dropped_dims)
 Given a tuple of dimension names, and either a collection of dimensions,
diff --git a/src/wrapper_array.jl b/src/wrapper_array.jl
index a7899df..e86fc2d 100644
--- a/src/wrapper_array.jl
+++ b/src/wrapper_array.jl
@@ -155,27 +155,25 @@ for f in (:getindex, :view, :dotview)
             return Base.$f(A, inds...)
         end
 
-        @propagate_inbounds function Base.$f(a::NamedDimsArray, inds::Vararg{<:Integer})
-            # Easy scalar case, will just return the element
-            return Base.$f(parent(a), inds...)
-        end
-
-        @propagate_inbounds function Base.$f(a::NamedDimsArray, ci::CartesianIndex)
-            # Easy scalar case, will just return the element
-            return Base.$f(parent(a), ci)
-        end
-
-        @propagate_inbounds function Base.$f(a::NamedDimsArray, inds...)
-            # Some nonscalar case, will return an array, so need to give that names.
+        @propagate_inbounds function Base.$f(a::NamedDimsArray, raw_inds...)
+            inds = Base.to_indices(parent(a), raw_inds)
             data = Base.$f(parent(a), inds...)
+            data isa AbstractArray || return data # Case of scalar output
             L = remaining_dimnames_from_indexing(dimnames(a), inds)
-            return NamedDimsArray{L}(data)
+            if L === ()
+                # Cases that merge dimensions down to vector like `mat[mat .> 0]`,
+                # and also zero-dimensional `view(mat, 1,1)`
+                return data
+            else
+                return NamedDimsArray{L}(data)
+            end
         end
     end
 end
 
 ############################################
 # setindex!
+
 @propagate_inbounds function Base.setindex!(a::NamedDimsArray, value; named_inds...)
     inds = order_named_inds(a; named_inds...)
     return setindex!(a, value, inds...)


Test patch:
diff --git a/test/name_core.jl b/test/name_core.jl
index 9cca305..b9647b2 100644
--- a/test/name_core.jl
+++ b/test/name_core.jl
@@ -112,13 +112,19 @@ end
 
 
 @testset "remaining_dimnames_from_indexing" begin
-    @test remaining_dimnames_from_indexing((:a, :b, :c), (10,20,30)) == tuple()
+    @test remaining_dimnames_from_indexing((:a, :b, :c), (10,20,30)) === ()
     @test remaining_dimnames_from_indexing((:a, :b, :c), (10, :,30)) == (:b,)
     @test remaining_dimnames_from_indexing((:a, :b, :c), (1:1, [true], [20])) == (:a, :b, :c)
 
     @test remaining_dimnames_from_indexing((:a, :b, :c), (:, [CartesianIndex()], :, :)) == (:a, :_, :b, :c)
     @test remaining_dimnames_from_indexing((:a, :b, :c), (1, [CartesianIndex()], 2, :)) == (:_, :c)
     @test remaining_dimnames_from_indexing((:a, :b, :c), (CartesianIndex(1,1), :)) == (:c,)
+
+    # Cases which drop dimension names
+    logical = Base.to_indices(ones(2,3), (rand(2,3).>0.5,))
+    @test remaining_dimnames_from_indexing((:a, :b), logical) === ()
+    cart_vec = findall(x -> x>0.5, rand(2,3))
+    @test remaining_dimnames_from_indexing((:a, :b), (cart_vec,)) === ()
 end
 @testset "allocations: remaining_dimnames_from_indexing" begin
     @test 0 == @ballocated (()->remaining_dimnames_from_indexing((:a, :b, :c), (:,390,:)))()
diff --git a/test/wrapper_array.jl b/test/wrapper_array.jl
index d33730a..d51cd32 100644
--- a/test/wrapper_array.jl
+++ b/test/wrapper_array.jl
@@ -45,7 +45,12 @@ end
     # Unspecified dims become slices
     @test nda[y=1] == nda[y=1, x=:] == nda[:, 1] == [10; 30]
 
-    @test_broken nda[CartesianIndex(1), 1] == nda[1, 1]
+    @test nda[CartesianIndex(1), 1] == nda[1, 1]
+
+    @testset "Views with all dims specified are not scalars" begin
+        @test view(nda, x=1, y=1) isa SubArray{Int, 0}
+        @test view(nda, 1,1) == view(nda.data, 1,1)
+    end
 
     @testset "name preservation" begin
         @test dimnames(nda[y=1]) == (:x, )
@@ -72,6 +77,46 @@ end
         @test size(nda[CartesianIndex(1, 1), newaxis]) == (1, )
         @test nda[CartesianIndex(1, 1), newaxis][1] == nda[1, 1]
     end
+
+    @testset "BitArray / Array{Bool}" begin
+        nda = NamedDimsArray(rand(1:9, 10,11,12), (:x, :y, :z))
+        ndv = NamedDimsArray(rand(1:9, 10), :x)
+
+        # one of several dimensions:
+        @test dimnames(nda[rand(10) .> 0.3, 1, :]) == (:x, :z)          # BitArray
+        @test dimnames(nda[collect(rand(10) .> 0.3), 1, :]) == (:x, :z) # Array{Bool}
+        @test dimnames(nda[ndv .> 3, 1, :]) == (:x, :z)                 # NamedDimsArray{...}
+        @test dimnames(nda[NamedDimsArray(collect(rand(10) .> 0.3), :x), 1, :]) == (:x, :z)
+
+        # only dim of a vector:
+        @test dimnames(ndv[rand(10) .> 0.3]) == (:x,)
+        @test dimnames(ndv[collect(rand(10) .> 0.3)]) == (:x,)
+        @test dimnames(ndv[ndv .> 3]) == (:x,)
+        @test dimnames(ndv[NamedDimsArray(collect(ndv .> 3), :x)]) == (:x,)
+
+        # https://github.com/invenia/NamedDims.jl/issues/70
+        # flattening of ndims>1 should drop names, as new dim is none of original ones:
+        @test nda[rand(10,11,12) .> 0.3] isa Vector
+        @test nda[collect(rand(10,11,12) .> 0.3)] isa Vector
+        @test nda[nda .> 3] isa Vector
+        @test nda[NamedDimsArray(collect(nda .> 3), dimnames(nda))] isa Vector
+    end
+
+    @testset "arrays of indices" begin
+        nda = NamedDimsArray(rand(1:9, 3,3,3), (:x, :y, :z))
+        ndv = NamedDimsArray(rand(1:9, 10), :x)
+
+        # integers
+        @test dimnames(nda[:, [1,3], :]) == (:x, :y, :z)
+        @test dimnames(nda[:, [1 3; 3 1], :]) == (:x, :_, :_, :z)
+
+        @test dimnames(ndv[[1, 3]]) == (:x,)
+        @test dimnames(ndv[[1 3; 3 1]]) == (:_, :_)
+
+        # Vector{CartesianIndex{N}}: for N>1 this makes a new dim, like nda[nda .> 3]
+        @test nda[findall(iseven, nda)] isa Vector
+        @test dimnames(ndv[findall(iseven, ndv)]) == (:x,)
+    end
 end