diff --git a/Project.toml b/Project.toml index 4bf406104..4cf11ff3a 100644 --- a/Project.toml +++ b/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" diff --git a/docs/src/api/reference.md b/docs/src/api/reference.md index 7c3bb7961..3ae6f72ff 100644 --- a/docs/src/api/reference.md +++ b/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 diff --git a/src/DimensionalData.jl b/src/DimensionalData.jl index ede289b04..d1e5450de 100644 --- a/src/DimensionalData.jl +++ b/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 diff --git a/src/Lookups/indexing.jl b/src/Lookups/indexing.jl index 5c86a1882..7ed7839bb 100644 --- a/src/Lookups/indexing.jl +++ b/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 diff --git a/src/Lookups/lookup_arrays.jl b/src/Lookups/lookup_arrays.jl index 7c254eb44..d126fcc6d 100644 --- a/src/Lookups/lookup_arrays.jl +++ b/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()) diff --git a/src/array/indexing.jl b/src/array/indexing.jl index 438eb8424..717a80645 100644 --- a/src/array/indexing.jl +++ b/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 diff --git a/test/indexing.jl b/test/indexing.jl index 6a804437c..f068f510a 100644 --- a/test/indexing.jl +++ b/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