Breaking: error on unordered or reverse indices

Project.toml

@@ -19,7 +19,6 @@ OrderedCollections = "bac558e1-5e72-5ebc-8fee-abe8a469f55d"
 PrecompileTools = "aea7be01-6a6a-4083-8856-8a6e6704d82a"
 Random = "9a3f8284-a2c9-5f02-9a11-845980a1fd5c"
 RecipesBase = "3cdcf5f2-1ef4-517c-9805-6587b60abb01"
-Statistics = "10745b16-79ce-11e8-11f9-7d13ad32a3b2"
 TableTraits = "3783bdb8-4a98-5b6b-af9a-565f29a5fe9c"
 Tables = "bd369af6-aec1-5ad0-b16a-f7cc5008161c"
 
@@ -128,6 +127,7 @@ Random = "9a3f8284-a2c9-5f02-9a11-845980a1fd5c"
 SafeTestsets = "1bc83da4-3b8d-516f-aca4-4fe02f6d838f"
 StatsBase = "2913bbd2-ae8a-5f71-8c99-4fb6c76f3a91"
 SparseArrays = "2f01184e-e22b-5df5-ae63-d93ebab69eaf"
+Statistics = "10745b16-79ce-11e8-11f9-7d13ad32a3b2"
 StatsPlots = "f3b207a7-027a-5e70-b257-86293d7955fd"
 Test = "8dfed614-e22c-5e08-85e1-65c5234f0b40"
 Unitful = "1986cc42-f94f-5a68-af5c-568840ba703d"

docs/src/api/reference.md

@@ -105,7 +105,8 @@ reorder
 ## Global lookup strictness settings
 
 Control how strict DimensionalData when comparing [`Lookup`](@ref)s
-before doing broadcasts and matrix multipications.
+before doing broadcasts, matrix multipications, or indexing with 
+possibly unordered `AbstractVector{Int}`.
 
 In some cases (especially `DimVector` and small `DimArray`) checking 
 lookup values match may be too costly compared to the operations.
@@ -117,6 +118,8 @@ DimensionalData.strict_broadcast
 DimensionalData.strict_broadcast!
 DimensionalData.strict_matmul
 DimensionalData.strict_matmul!
+DimensionalData.strict_order
+DimensionalData.strict_order!
 ```
 
 ## Base methods

src/DimensionalData.jl

@@ -39,7 +39,8 @@ using .Dimensions: StandardIndices, DimOrDimType, DimTuple, DimTupleOrEmpty, Dim
 using .Dimensions: INTERFACE_QUERY_FUNCTION_NAMES
 import .Lookups: metadata, set, _set, rebuild, basetypeof, 
     order, span, sampling, locus, val, bounds, intervalbounds,
-    hasselection, units, SelectorOrInterval, Begin, End
+    hasselection, units, SelectorOrInterval, Begin, End,
+    strict_order, strict_order!
 import .Dimensions: dims, refdims, name, lookup, kw2dims, hasdim, label, _astuple
 
 using OrderedCollections: OrderedDict

src/Lookups/indexing.jl

@@ -1,4 +1,31 @@
 
+const STRICT_ORDER_CHECKS = Ref(true)
+const STRICT_ORDER_DOCS = """
+By default indexing with Vector{Int} is required 
+to be ordered if lookups are ordered to avoid broken lookups.
+`@inbounds` annotations skip this check, but in some cases where 
+outputs are not used you may wish to disable it completely.
+"""
+
+"""
+    strict_order()
+
+Check if strick broadcasting checks are active.
+
+$STRICT_ORDER_DOCS
+"""
+strict_order() = STRICT_ORDER_CHECKS[]
+
+"""
+    strict_order!(x::Bool)
+
+Set global AbstractVector{Int} indexing checks to `strict`, or not, for all `AbstractDimArray`.
+
+$STRICT_ORDER_DOCS
+"""
+strict_order!(x::Bool) = STRICT_ORDER_CHECKS[] = x
+
+
 for f in (:getindex, :view, :dotview)
     @eval begin
         # Int and CartesianIndex forward to the parent
@@ -7,11 +34,45 @@ for f in (:getindex, :view, :dotview)
         # AbstractArray, Colon and CartesianIndices: the lookup is rebuilt around a new parent
         @propagate_inbounds Base.$f(l::Lookup, i::Union{AbstractArray,Colon}) = 
             rebuild(l; data=Base.$f(parent(l), i))
+        # span may need its step size or bounds updated
+        @propagate_inbounds function Base.$f(l::AbstractSampled, i::AbstractArray)
+            i1 = Base.to_indices(l, (i,))[1]
+            rebuild(l; data=Base.$f(parent(l), i1), span=slicespan(l, i1))
+        end
+        # With ordered lookups AbstractArray{Integer} needs to be ordered
+        @propagate_inbounds function Base.$f(l::AbstractSampled, i::AbstractArray{<:Integer})
+            @boundscheck checkorder(l, i)
+            i1 = only(Base.to_indices(l, (i,)))
+            rebuild(l; data=Base.$f(parent(l), i1), span=slicespan(l, i1))
+        end
+        @propagate_inbounds function Base.$f(l::AbstractCategorical, i::AbstractArray{<:Integer})
+            @boundscheck checkorder(l, i)
+            rebuild(l; data=Base.$f(parent(l), i))
+        end
+        # Indexing AbstractCyclic with `AbstractArray` must rebuild the lookup as
+        # `Sampled` as we no longer have the whole cycle.
+        @propagate_inbounds function Base.$f(l::AbstractCyclic, i::AbstractArray{<:Integer})
+            @boundscheck checkorder(l, i)
+            Sampled(rebuild(l; data=Base.$f(parent(l), i)))
+        end
         # Selector gets processed with `selectindices`
-        @propagate_inbounds Base.$f(l::Lookup, i::SelectorOrInterval) = Base.$f(l, selectindices(l, i))
+        @propagate_inbounds Base.$f(l::Lookup, i::SelectorOrInterval) = 
+            Base.$f(l, selectindices(l, i))
+        # Everything else we just index the parent and check if the result is an array
         @propagate_inbounds function Base.$f(l::Lookup, i)
             x = Base.$f(parent(l), i)
             x isa AbstractArray ? rebuild(l; data=x) : x
         end
     end
 end
+
+function checkorder(l, i)
+    if strict_order() && isordered(l)
+        issorted(i) || throw(ArgumentError("""
+            For `ForwardOrdered` or `ReverseOrdered` lookups, indices of `AbstractVector{Int}` must be in ascending order. 
+            Use `@inbounds` to avoid this check locally, or `DimensionalData.strict_order!(false)` globally.
+        """))
+    end
+end
+# Avoid checks for Bool, Bool indexing is ordered
+checkorder(l, i::AbstractArray{Bool}) = nothing

src/Lookups/lookup_arrays.jl

@@ -188,16 +188,6 @@ function Base.:(==)(l1::AbstractSampled, l2::AbstractSampled)
     parent(l1) == parent(l2)
 end
 
-for f in (:getindex, :view, :dotview)
-    @eval begin
-        # span may need its step size or bounds updated
-        @propagate_inbounds function Base.$f(l::AbstractSampled, i::AbstractArray)
-            i1 = Base.to_indices(l, (i,))[1]
-            rebuild(l; data=Base.$f(parent(l), i1), span=slicespan(l, i1))
-        end
-    end
-end
-
 function Adapt.adapt_structure(to, l::AbstractSampled)
     rebuild(l; data=Adapt.adapt(to, parent(l)), metadata=NoMetadata(), span=Adapt.adapt(to, span(l)))
 end
@@ -345,13 +335,6 @@ cycle_status(l::AbstractCyclic) = l.cycle_status
 
 bounds(l::AbstractCyclic{<:Any,T}) where T = (typemin(T), typemax(T))
 
-# Indexing with `AbstractArray` must rebuild the lookup as
-# `Sampled` as we no longer have the whole cycle.
-for f in (:getindex, :view, :dotview)
-    @eval @propagate_inbounds Base.$f(l::AbstractCyclic, i::AbstractArray) =
-        Sampled(rebuild(l; data=Base.$f(parent(l), i)))
-end
-
 
 no_cycling(l::AbstractCyclic) = rebuild(l; cycle_status=NotCycling())
 

src/array/indexing.jl

@@ -116,8 +116,8 @@ for f in (:getindex, :view, :dotview)
     # Special case zero dimensional arrays being indexed with missing dims
     if f == :getindex
         # Catch this before the dimension is converted to ()
-        @eval $_dim_f(A::AbstractDimArray{<:Any,0}) = rebuild(A, fill(A[]))
-        @eval function $_dim_f(A::AbstractDimArray{<:Any,0}, d1::Dimension, ds::Dimension...)
+        @eval @propagate_inbounds $_dim_f(A::AbstractDimArray{<:Any,0}) = rebuild(A, fill(A[]))
+        @eval @propagate_inbounds function $_dim_f(A::AbstractDimArray{<:Any,0}, d1::Dimension, ds::Dimension...)
             Dimensions._extradimswarn((d1, ds...))
             return rebuild(A, fill(A[]))
         end

test/indexing.jl

@@ -266,8 +266,8 @@ end
         @test bounds(a) == ((143.0, 145.0), (-38.0, -36.0))
         @test bounds(a, X) == (143.0, 145.0)
 
-        a = da[X([2, 1]), Y([2, 1])] # Indexing with array works
-        @test a == [4 3; 2 1]
+        # Indexing with array works
+        @test da[X([1, 2]), Y([1, 2])] == [1 2; 3 4]
     end
 
     @testset "dimindices and dimselectors" begin
@@ -808,3 +808,39 @@ end
         @test Base.checkindex(Bool, 1:10, Begin:2:8)
     end
 end
+
+@testset "order breaking indexing" begin
+    @testset "on Ordered dimensions" begin
+        A = DimArray(reshape(1:12, 3, 4), (X(Sampled(3:-1:1; order=ReverseOrdered())), Y(10:10:40)))
+        @testset "Sorted Int vectors works" begin
+            @test A[X([1, 3]), Y([2, 4])] == [4 10; 6 12]
+        end
+        @testset "Unsorted Int vector throws ArgumentError" begin
+            @test_throws ArgumentError A[X([1, 3]), Y([4, 2])]
+            @test_throws ArgumentError A[X([3, 1]), Y([2, 4])]
+        end
+        @testset "@inbounds skips order checks" begin
+            f(A, i) = (@inbounds A[i])
+            @test_nowarn f(A, (X([1, 3]), Y([4, 2])))
+            @test_nowarn f(A, (X([3, 1]), Y([2, 4])))
+        end
+        @testset "strict_order!(false) skips order checks" begin
+            DimensionalData.strict_order!(false)
+            @test DimensionalData.strict_order() == false
+            @test_nowarn A[(X([1, 3]), Y([4, 2]))]
+            @test_nowarn A[(X([3, 1]), Y([2, 4]))]
+            DimensionalData.strict_order!(true)
+            @test DimensionalData.strict_order()
+        end
+    end
+
+    @testset "on Unordered dimension" begin
+        A = DimArray(reshape(1:12, 3, 4), (X(Sampled([3, 1, 2]; order=Unordered())), Y(10:10:40)))
+        @testset "Unsorted Int vector on Unordered works" begin
+            @test A[X([3, 1, 2]), Y([2, 4])] == [6 12; 4 10; 5 11]
+        end
+        @testset "Unsorted Int vector on Ordered throws ArgumentError" begin
+            @test_throws ArgumentError A[X([1, 3]), Y([4, 1, 2])]
+        end
+    end
+end