Better promotedims (#799)

* better promotion of sampled and categorical with different orders * removed accidental * removed accidental * more better promotedims rules and tests * fix ambiguities
rafaqz · Sep 14, 2024 · caac174 · caac174
1 parent 789a15a
commit caac174
Show file tree

Hide file tree

Showing 3 changed files with 106 additions and 31 deletions.
diff --git a/src/Lookups/lookup_arrays.jl b/src/Lookups/lookup_arrays.jl
@@ -864,51 +864,80 @@ promote_first(l1::C, ls::C...) where C<:AbstractCategorical = l1
 promote_first(l1::C, ::C, ::C...) where C<:AbstractCategorical = rebuild(l1; metadata=NoMetadata())
 function promote_first(l1::AbstractCategorical, l2::AbstractCategorical, ls::AbstractCategorical...)
     ls = (l2, ls...)
-    all(map(l -> order(l) == order(l1), ls)) || return NoLookup(Base.OneTo(length(l1)))
+    o = all(map(l -> order(l) == order(l1), ls)) ? order(l1) : Unordered()
     data = promote_first(parent(l1), map(parent, ls)...)
+    # Check we have all the same type of AbstractCategorical
     if all(map(l -> basetypeof(l) == basetypeof(l1), ls))
-        return rebuild(l1; data, metadata=NoMetadata())
-    else # Fall back to standard Categorical
-        return Categorical(data; order=order(l1), metadata=NoMetadata())
+        return rebuild(l1; data, order=o, metadata=NoMetadata())
+    else # Otherwise fall back to Categorical
+        return Categorical(data; order=o, metadata=NoMetadata())
     end
 end
 promote_first(l1::AbstractSampled) = l1
 promote_first(l1::S, ::S, ::S...) where S<:AbstractSampled = l1
 function promote_first(l1::AbstractSampled, l2::AbstractSampled, ls::AbstractSampled...)
     ls = (l2, ls...)
-    all(map(l -> order(l) == order(l1), ls)) &&
-        all(map(l -> typeof(sampling(l)) == typeof(sampling(l1)), ls))  &&
-        all(map(l -> basetypeof(span(l)) == basetypeof(span(l1)), ls)) || 
+
+    # We cant always convert explicit to something else
+    if any(map(isexplicit, (l1, ls...))) && !all(isexplicit, (l1, ls...))
         return NoLookup(Base.OneTo(length(l1)))
+    end
 
     data = promote_first(parent(l1), map(parent, ls)...)
+    sa = promote_first(sampling(l1), map(sampling, ls)...)
     kw = (;
-        order=order(l1),
-        span=promote_first(span(l1), map(span, ls)...),
-        sampling=sampling(l1),
+        order=promote_first(order(l1), map(order, ls)...),
+        sampling=sa,
+        span=promote_first(l1, sa, span(l1), map(span, ls)...),
         metadata=NoMetadata(),
     )
+    # Check we have all the same type of AbstractSampled
     if all(map(l -> basetypeof(l) == basetypeof(l1), ls))
         return rebuild(l1; data, kw...)
-    else
+    else # Otherwise fall back to Sampled
         return Sampled(data; kw...)
     end
 end
+
+# Order
+# Only matching Order remain the same
+promote_first(::O, ::O...) where O<:Order = O()
+# Everthing else is Unordered
+promote_first(::Order, ::Order...) = Unordered()
+
+# Sampling 
+# Only matching locus Intervals remain Intervals
+promote_first(i1::I, ::I...) where I<:Intervals = i1
+# Any other mix is Points
+promote_first(::Sampling, ::Sampling...) = Points() 
+
 # Span
-function promote_first(s::Regular, ss::Regular...) 
+# Regular remains regular, eltype is promoted
+function promote_first(::Lookup, ::Sampling, s::Regular, ss::Regular...) 
     T = promote_type(typeof(val(s)), map(typeof ∘ val, ss)...)
     Regular(convert(T, val(s)))
 end
-promote_first(a::T, b::T...) where T<:Irregular = a 
+# # Matching irregular is returns
+promote_first(::Lookup, ::Sampling, a::T, b::T...) where T<:Irregular = a 
+# # Number and DateTime are promoted
 for E in (Base.Number, Dates.AbstractTime)
-    @eval function promote_first(
+    @eval function promote_first(::Lookup,
         s::Irregular{Tuple{<:$E,<:$E}}, ss::Irregular{Tuple{<:$E,<:$E}}...
     )
         T = promote_type(maps(s -> promote_type(typeof(val(s)[1]), typeof(val(s)[2])), (s, ss...))...)
         return Irregular(convert(T, val(a)[1]), convert(T, val(a)[2]))
     end
 end
-promote_first(::Irregular, ::Irregular...) = Irregular((nothing, nothing))
+# Explicit promotes its matrix
+promote_first(::Lookup, ::Sampling, s1::Explicit, ss::Explicit...) = 
+    Explicit(promote_first(val(s1), map(val, ss)...))
+# Mixed Regular/Irregular always become Irregular
+promote_first(l::Lookup, sampling::Sampling, ::Union{Regular,Irregular}, ::Union{Regular,Irregular}...) = 
+    _irregular(sampling, l)
+
+_irregular(::Points, l) = Irregular(nothing, nothing)
+_irregular(::Intervals, l) = Irregular(bounds(l))
+
 # Data
 promote_first(a1::A) where A<:AbstractArray = a1
 promote_first(a1::A, ::A, ::A...) where A<:AbstractArray = a1

diff --git a/src/Lookups/predicates.jl b/src/Lookups/predicates.jl
@@ -3,6 +3,8 @@ isregular(::Regular) = true
 isregular(::Sampling) = false
 isexplicit(::Explicit) = true
 isexplicit(::Sampling) = false
+isaligned(::Lookup) = false
+isaligned(::Aligned) = true
 issampled(::AbstractSampled) = true
 issampled(::Lookup) = false
 iscategorical(::AbstractCategorical) = true

diff --git a/test/primitives.jl b/test/primitives.jl
@@ -548,23 +548,67 @@ end
 @testset "promotedims" begin
     nl = NoLookup(Base.OneTo(2))
     c = Categorical(["a", "b"]; order=ForwardOrdered())
+    c_unord = Categorical(["a", "b"]; order=Unordered())
     c_sub = Categorical([view("a", 1:1), view("b", 1:1)]; order=ForwardOrdered())
     s = Sampled(1.0:1.0:2.0; span=Regular(1.0), sampling=Points(), order=ForwardOrdered())
     s_int = Sampled(1:1:2; span=Regular(1), sampling=Points(), order=ForwardOrdered())
-    @test promotedims(X(nl), X(c)) === promotedims(X(c), X(nl)) === X(nl)
-    @test promotedims(X(nl), X(s)) === promotedims(X(s), X(nl)) === X(nl)
-    @test promotedims(X(c), X(s)) === promotedims(X(s), X(c)) === X(nl)
-    @test promotedims(X(s), X(s)) === X(s)
-    @test promotedims(X(s_int), X(s_int)) === X(s_int)
-    @test promotedims(X(s), X(s_int)) === promotedims(X(s_int), X(s)) === X(s)
-    @test promotedims(X(c), X(c_sub)) == promotedims(X(c_sub), X(c)) == X(c)
-    @test eltype(promotedims(X(c_sub), X(c))) == String
-    @test promotedims((X(c), Y(s)), (X(c), Y(s))) == (X(c), Y(s))
-    @test promotedims((X(c), Y(nl)), (X(nl), Y(s))) == (X(nl), Y(nl))
-    @test promotedims((X(c), Y(s_int)), (X(c_sub), Y(s))) == (X(c), Y(s))
-
-    @test promotedims(X(1), X(1.0)) == promotedims(X(1.0), X(1)) == X(1.0)
-    @test promotedims(X("a"), X(view("a", 1:1))) ==
-          promotedims(X(view("a", 1:1)), X("a")) == 
-          X("a")
+    s_intervals = Sampled(1.0:1.0:2.0; span=Regular(1.0), sampling=Intervals(), order=ForwardOrdered())
+    s_unord = Sampled(1.0:1.0:2.0; span=Regular(1.0), sampling=Points(), order=Unordered())
+    s_irreg = Sampled(1.0:1.0:2.0; span=Irregular((nothing, nothing)), sampling=Points(), order=ForwardOrdered())
+    s_exp = Sampled(1.0:1.0:2.0; span=Explicit(), sampling=Intervals(Start()), order=ForwardOrdered())
+    s_exp_int = Sampled(1:1:2; span=Explicit(), sampling=Intervals(Start()), order=ForwardOrdered())
+
+    @testset "promote withe same is no change" begin
+        @test promotedims(X(s), X(s)) === X(s)
+        @test promotedims(X(s_int), X(s_int)) === X(s_int)
+    end
+
+    @testset "mixing returns NoLookup" begin
+        @test promotedims(X(nl), X(c)) === promotedims(X(c), X(nl)) === X(nl)
+        @test promotedims(X(nl), X(s)) === promotedims(X(s), X(nl)) === X(nl)
+        @test promotedims(X(c), X(s)) === promotedims(X(s), X(c)) === X(nl)
+    end
+
+    @testset "eltype: Float64 wins in Real" begin
+        @test promotedims(X(s), X(s_int)) === promotedims(X(s_int), X(s)) === X(s)
+    end
+
+    @testset "eltype: String wins in AbstractString" begin
+        @test promotedims(X(c), X(c_sub)) == promotedims(X(c_sub), X(c)) == X(c)
+        @test eltype(promotedims(X(c_sub), X(c))) == String
+    end
+
+    @testset "order: Unordered wins" begin
+        @test promotedims(X(s), X(s_unord)) == X(s_unord)
+        @test promotedims(X(c), X(c_unord)) == X(c_unord)
+    end
+
+    @testset "sampling: Points wins" begin
+        @test promotedims(X(s), X(s_intervals)) == X(s)
+        @test promotedims(X(s_intervals), X(s)) == X(s)
+    end
+
+    @testset "span: Irregular wins" begin
+        @test promotedims(X(s), X(s_irreg)) == X(s_irreg)
+        @test promotedims(X(s_irreg), X(s)) == X(s_irreg)
+    end
+
+    @testset "span: Explicit must be on its own" begin
+        @test promotedims(X(s), X(s_exp)) == X(nl)
+        @test promotedims(X(s_exp), X(s_exp)) == X(s_exp)
+        @test promotedims(X(s_exp), X(s_exp_int)) == X(s_exp)
+        @test promotedims(X(s_exp_int), X(s_exp)) == X(s_exp)
+    end
+
+    @testset "tuples of dims" begin
+        @test promotedims((X(c), Y(s)), (X(c), Y(s))) == (X(c), Y(s))
+        @test promotedims((X(c), Y(nl)), (X(nl), Y(s))) == (X(nl), Y(nl))
+        @test promotedims((X(c), Y(s_int)), (X(c_sub), Y(s))) == (X(c), Y(s))
+    end
+
+    @testset "non-Lookup val" begin
+        @test promotedims(X(1), X(1.0)) == promotedims(X(1.0), X(1)) == X(1.0)
+        @test promotedims(X("a"), X(view("a", 1:1))) ==
+            promotedims(X(view("a", 1:1)), X("a")) == X("a")
+    end
 end