Repository: JuliaDynamics/ComplexityMeasures.jl (julia)

Issue:
Missing deprecation for `OrdinalPatterns{m}(; τ)`
During the rewrite for 3.0, the keyword `τ` became a positional argument. Therefore, pre-V3 code like the following fails

```julia
julia> m = 3; τ = 2; OrdinalPatterns{m}(; τ)
ERROR: MethodError: no method matching (OrdinalPatterns{3})(; τ::Int64)

Closest candidates are:
  (OrdinalPatterns{m})() where m got unsupported keyword argument "τ"
   @ ComplexityMeasures ~/Documents/Repos/ComplexityMeasures.jl/src/outcome_spaces/ordinal_patterns.jl:198
  (OrdinalPatterns{m})(::Int64) where m got unsupported keyword argument "τ"
   @ ComplexityMeasures ~/Documents/Repos/ComplexityMeasures.jl/src/outcome_spaces/ordinal_patterns.jl:198
  (OrdinalPatterns{m})(::Int64, ::F) where {m, F} got unsupported keyword argument "τ"
   @ ComplexityMeasures ~/Documents/Repos/ComplexityMeasures.jl/src/outcome_spaces/ordinal_patterns.jl:198

Stacktrace:
 [1] top-level scope
   @ REPL[88]:1
```

Whether this was intentional or not, it needs a deprecation.

Pull request:
Fixes #372. 

I also parameterized the `τ` field of the structs explicitly. These were previously hard-coded as `Int`, but there's no reason why we should be so strict.

Patch:
diff --git a/src/deprecations.jl b/src/deprecations.jl
index 354f1f196..b03489319 100644
--- a/src/deprecations.jl
+++ b/src/deprecations.jl
@@ -99,17 +99,17 @@ function OrdinalPatternEncoding(m::Int, lt::F = isless_rand) where {F}
     return OrdinalPatternEncoding{m, F}(zero(MVector{m, Int}), lt)
 end
 # Initializations
-function OrdinalPatterns(; τ::Int = 1, m::Int = 3, lt::F=isless_rand) where {F}
+function OrdinalPatterns(; τ::I = 1, m::Int = 3, lt::F=isless_rand) where {F, I}
     m >= 2 || throw(ArgumentError("Need order m ≥ 2."))
-    return OrdinalPatterns{m, F}(OrdinalPatternEncoding{m}(lt), τ)
+    return OrdinalPatterns{m, F, I}(OrdinalPatternEncoding{m}(lt), τ)
 end
-function WeightedOrdinalPatterns(; τ::Int = 1, m::Int = 3, lt::F=isless_rand) where {F}
+function WeightedOrdinalPatterns(; τ::I = 1, m::Int = 3, lt::F=isless_rand) where {F, I}
     m >= 2 || throw(ArgumentError("Need order m ≥ 2."))
-    return WeightedOrdinalPatterns{m, F}(OrdinalPatternEncoding{m}(lt), τ)
+    return WeightedOrdinalPatterns{m, F, I}(OrdinalPatternEncoding{m}(lt), τ)
 end
-function AmplitudeAwareOrdinalPatterns(; A = 0.5, τ::Int = 1, m::Int = 3, lt::F=isless_rand) where {F}
+function AmplitudeAwareOrdinalPatterns(; A::T = 0.5, τ::I = 1, m::Int = 3, lt::F=isless_rand) where {F, I, T}
     m >= 2 || throw(ArgumentError("Need order m ≥ 2."))
-    return AmplitudeAwareOrdinalPatterns{m, F}(OrdinalPatternEncoding{m}(lt), τ, A)
+    return AmplitudeAwareOrdinalPatterns{m, F, I, T}(OrdinalPatternEncoding{m}(lt), τ, A)
 end
 
 # For 3.0
diff --git a/src/outcome_spaces/ordinal_patterns.jl b/src/outcome_spaces/ordinal_patterns.jl
index 02d23ad1a..c3b2146ad 100644
--- a/src/outcome_spaces/ordinal_patterns.jl
+++ b/src/outcome_spaces/ordinal_patterns.jl
@@ -92,9 +92,9 @@ x = StateSpaceSet(rand(N, m)) # some input dataset
 p = probabilities!(πs_ts, est, x)
 ```
 """
-struct OrdinalPatterns{M,F} <: OrdinalOutcomeSpace{M}
+struct OrdinalPatterns{M, F, I <: Integer} <: OrdinalOutcomeSpace{M}
     encoding::OrdinalPatternEncoding{M,F}
-    τ::Int
+    τ::I
 end
 
 # Explicitly implement `counts`, because decoding outcomes is expensive.
@@ -155,9 +155,9 @@ of the same pattern.
     correctly for each vector of the input dataset (which may be a delay-embedded
     timeseries).
 """
-struct WeightedOrdinalPatterns{M,F} <: OrdinalOutcomeSpace{M}
-    encoding::OrdinalPatternEncoding{M,F}
-    τ::Int
+struct WeightedOrdinalPatterns{M, F, I <: Integer} <: OrdinalOutcomeSpace{M}
+    encoding::OrdinalPatternEncoding{M, F}
+    τ::I
 end
 
 is_counting_based(o::WeightedOrdinalPatterns) = false
@@ -186,26 +186,107 @@ elements of
 ``\\mathbf{x}_i`` are weighted. Only mean amplitude of the state vector
 elements are weighted when ``A=1``. With, ``0<A<1``, a combined weighting is used.
 """
-struct AmplitudeAwareOrdinalPatterns{M,F} <: OrdinalOutcomeSpace{M}
-    encoding::OrdinalPatternEncoding{M,F}
-    τ::Int
-    A::Float64
+struct AmplitudeAwareOrdinalPatterns{M, F, I<:Integer, T<:Real} <: OrdinalOutcomeSpace{M}
+    encoding::OrdinalPatternEncoding{M, F}
+    τ::I
+    A::T
 end
 
 is_counting_based(o::AmplitudeAwareOrdinalPatterns) = false
 
-# Initializations
-function OrdinalPatterns{m}(τ::Int = 1, lt::F=isless_rand) where {m, F}
+# Initializations (also handles deprecations)
+function OrdinalPatterns{m}(τ = 1, lt = isless_rand; kwargs...) where {m}
+    if haskey(kwargs, :τ)
+        msg = "Keyword argument `τ` to `OrdinalPatterns` is deprecated. " *
+        "The signature is now " * 
+        "`OrdinalPatterns{m}(τ = 1, lt::Function = ComplexityMeasures.isless_rand)`" * 
+        ", so provide `τ` as a positional argument instead. " * 
+        "In this call, the given keyword `τ` is used instead of the positional `τ`."
+        @warn msg
+        τ = kwargs[:τ]
+    end
+    if haskey(kwargs, :lt)
+        msg = "Keyword argument `lt` to `OrdinalPatterns` is deprecated. " *
+        "The signature is now " * 
+        "`OrdinalPatterns{m}(τ = 1, lt::Function = ComplexityMeasures.isless_rand)`" * 
+        ", so provide `lt` as a positional argument instead. "  * 
+        "In this call, the given keyword `lt` is used instead of the positional `lt`."
+        @warn msg
+        lt = kwargs[:lt]
+    end
+
     m >= 2 || throw(ArgumentError("Need order m ≥ 2."))
-    return OrdinalPatterns{m, F}(OrdinalPatternEncoding{m}(lt), τ)
+    F, I = typeof(lt), typeof(τ)
+    return OrdinalPatterns{m, F, I}(OrdinalPatternEncoding{m}(lt), τ)
 end
-function WeightedOrdinalPatterns{m}(τ::Int = 1, lt::F=isless_rand) where {m, F}
+
+function WeightedOrdinalPatterns{m}(τ = 1, lt = isless_rand; kwargs...) where {m}
+    if haskey(kwargs, :τ)
+        msg = "Keyword argument `τ` to `WeightedOrdinalPatterns` is deprecated. " *
+        "The signature is now " * 
+        "`WeightedOrdinalPatterns{m}(τ::Int = 1, lt::F=ComplexityMeasures.isless_rand)`" * 
+        ", so provide `τ` as a positional argument instead. "  * 
+        "In this call, the given keyword `τ` is used instead of the positional `τ`."
+        @warn msg
+        τ = kwargs[:τ]
+    end
+    if haskey(kwargs, :lt)
+        msg = "Keyword argument `lt` to `WeightedOrdinalPatterns` is deprecated. " *
+        "The signature is now " * 
+        "`WeightedOrdinalPatterns{m}(τ = 1, lt::Function = ComplexityMeasures.isless_rand)`" * 
+        ", so provide `lt` as a positional argument instead. "  * 
+        "In this call, the given keyword `lt` is used instead of the positional `lt`."
+        @warn msg
+        lt = kwargs[:lt]
+    end
+
     m >= 2 || throw(ArgumentError("Need order m ≥ 2."))
-    return WeightedOrdinalPatterns{m, F}(OrdinalPatternEncoding{m}(lt), τ)
+    F, I = typeof(lt), typeof(τ)
+
+    return WeightedOrdinalPatterns{m, F, I}(OrdinalPatternEncoding{m}(lt), τ)
 end
-function AmplitudeAwareOrdinalPatterns{m}(τ::Int = 1, A = 0.5, lt::F=isless_rand) where {m, F}
+
+function AmplitudeAwareOrdinalPatterns{m}(τ = 1, A = 0.5, lt = isless_rand; 
+        kwargs...) where {m}
+    # because the order of the arguments is different from the other ordinal outcome spaces
+    if A isa Function
+        msg = "Second argument to `AmplitudeAwareOrdinalPatterns` must be a function. " *
+        "Got a $(typeof(A)).";
+        throw(ArgumentError(msg))
+    end
+    
+    if haskey(kwargs, :τ)
+        msg = "Keyword argument `τ` to `AmplitudeAwareOrdinalPatterns` is deprecated. " *
+        "The signature is now " * 
+        "`AmplitudeAwareOrdinalPatterns{m}(τ::Int = 1, A = 0.5, lt::F=isless_rand)`" * 
+        ", so provide `τ` as a positional argument instead. "  * 
+        "In this call, the given keyword `τ` is used instead of the positional `τ`."
+        @warn msg
+        τ = kwargs[:τ]
+    end
+    if haskey(kwargs, :lt)
+        msg = "Keyword argument `lt` to `AmplitudeAwareOrdinalPatterns` is deprecated. " *
+        "The signature is now " * 
+        "`AmplitudeAwareOrdinalPatterns{m}(τ::Int = 1, A = 0.5, lt::F=isless_rand)`" * 
+        ", so provide `lt` as a positional argument instead. "  * 
+        "In this call, the given keyword `lt` is used instead of the positional `lt`."
+        @warn msg
+        lt = kwargs[:lt]
+    end
+    if haskey(kwargs, :A)
+        msg = "Keyword argument `A` to `AmplitudeAwareOrdinalPatterns` is deprecated. " *
+        "The signature is now " * 
+        "`AmplitudeAwareOrdinalPatterns{m}(τ::Int = 1, A = 0.5, lt::F=isless_rand)`" * 
+        ", so provide `A` as a positional argument instead. "  * 
+        "In this call, the given keyword `A` is used instead of the positional `A`."
+        @warn msg
+        A = kwargs[:A]
+    end
+
     m >= 2 || throw(ArgumentError("Need order m ≥ 2."))
-    return AmplitudeAwareOrdinalPatterns{m, F}(OrdinalPatternEncoding{m}(lt), τ, A)
+    encoding = OrdinalPatternEncoding{m}(lt)
+    F, I, T = typeof(lt), typeof(τ), typeof(A)
+    return AmplitudeAwareOrdinalPatterns{m, F, I, T}(encoding, τ, A)
 end
 
 ###########################################################################################


Test patch:
diff --git a/test/deprecations.jl b/test/deprecations.jl
index 4557a0bcb..656943405 100644
--- a/test/deprecations.jl
+++ b/test/deprecations.jl
@@ -41,10 +41,102 @@ end
 
     @test entropy_normalized(Shannon(MathConstants.e), ValueBinning(4), x) ==
         information_normalized(Shannon(MathConstants.e), ValueBinning(4), x)
-
-    @test SymbolicPermutation() isa OrdinalPatterns
-    @test SymbolicWeightedPermutation() isa WeightedOrdinalPatterns
-    @test SymbolicAmplitudeAwarePermutation() isa AmplitudeAwareOrdinalPatterns
+    
+    @testset "Ordinal pattern constructiors" begin
+        @test SymbolicPermutation() isa OrdinalPatterns
+        @test SymbolicWeightedPermutation() isa WeightedOrdinalPatterns
+        @test SymbolicAmplitudeAwarePermutation() isa AmplitudeAwareOrdinalPatterns
+
+        @testset "OrdinalPatterns" begin 
+            msg = "Keyword argument `τ` to `OrdinalPatterns` is deprecated. " *
+            "The signature is now " * 
+            "`OrdinalPatterns{m}(τ = 1, lt::Function = ComplexityMeasures.isless_rand)`" * 
+            ", so provide `τ` as a positional argument instead. " * 
+            "In this call, the given keyword `τ` is used instead of the positional `τ`."
+            τ = 1; 
+            @test_logs (:warn, msg) OrdinalPatterns{3}(τ + 1; τ)
+
+            msg = "Keyword argument `lt` to `OrdinalPatterns` is deprecated. " *
+            "The signature is now " * 
+            "`OrdinalPatterns{m}(τ = 1, lt::Function = ComplexityMeasures.isless_rand)`" * 
+            ", so provide `lt` as a positional argument instead. "  * 
+            "In this call, the given keyword `lt` is used instead of the positional `lt`."
+            lt = Base.isless;
+            @test_logs (:warn, msg) OrdinalPatterns{3}(τ, lt; lt)
+
+            # Test that keyword argument is preferred over positional argument
+            o = OrdinalPatterns{3}(5; τ = 2)
+            @test o.τ == 2
+            lt = Base.isless;
+            ltr = ComplexityMeasures.isless_rand
+            o = OrdinalPatterns{3}(2, lt; lt = ltr)
+            @test o.encoding.lt == ltr
+        end
+
+        @testset "WeightedOrdinalPatterns" begin 
+            msg = "Keyword argument `τ` to `WeightedOrdinalPatterns` is deprecated. " *
+            "The signature is now " * 
+            "`WeightedOrdinalPatterns{m}(τ::Int = 1, lt::F=ComplexityMeasures.isless_rand)`" * 
+            ", so provide `τ` as a positional argument instead. "  * 
+            "In this call, the given keyword `τ` is used instead of the positional `τ`."
+            τ = 1; 
+            @test_logs (:warn, msg) WeightedOrdinalPatterns{3}(τ + 1; τ)
+
+            msg = "Keyword argument `lt` to `WeightedOrdinalPatterns` is deprecated. " *
+            "The signature is now " * 
+            "`WeightedOrdinalPatterns{m}(τ = 1, lt::Function = ComplexityMeasures.isless_rand)`" * 
+            ", so provide `lt` as a positional argument instead. "  * 
+            "In this call, the given keyword `lt` is used instead of the positional `lt`."
+            lt = Base.isless;
+            @test_logs (:warn, msg) WeightedOrdinalPatterns{3}(τ, lt; lt)
+
+            # Test that keyword argument is preferred over positional argument
+            o = WeightedOrdinalPatterns{3}(2; τ)
+            @test o.τ == WeightedOrdinalPatterns{3}(τ).τ
+            lt = Base.isless;
+            ltr = ComplexityMeasures.isless_rand
+            @test WeightedOrdinalPatterns{3}(2, lt; lt = ltr).encoding.lt == ltr
+        end
+
+        @testset "AmplitudeAwareOrdinalPatterns" begin 
+            msg = "Keyword argument `τ` to `AmplitudeAwareOrdinalPatterns` is deprecated. " *
+            "The signature is now " * 
+            "`AmplitudeAwareOrdinalPatterns{m}(τ::Int = 1, A = 0.5, lt::F=isless_rand)`" * 
+            ", so provide `τ` as a positional argument instead. "  * 
+            "In this call, the given keyword `τ` is used instead of the positional `τ`."
+            τ = 1; 
+            @test_logs (:warn, msg) AmplitudeAwareOrdinalPatterns{3}(τ + 1; τ)
+
+            msg = "Keyword argument `lt` to `AmplitudeAwareOrdinalPatterns` is deprecated. " *
+            "The signature is now " * 
+            "`AmplitudeAwareOrdinalPatterns{m}(τ::Int = 1, A = 0.5, lt::F=isless_rand)`" * 
+            ", so provide `lt` as a positional argument instead. "  * 
+            "In this call, the given keyword `lt` is used instead of the positional `lt`."
+            lt = Base.isless;
+            @test_logs (:warn, msg) AmplitudeAwareOrdinalPatterns{3}(τ, 0.5, lt; lt)
+            @test_throws ArgumentError AmplitudeAwareOrdinalPatterns{3}(τ, lt; lt)
+
+            msg = "Keyword argument `A` to `AmplitudeAwareOrdinalPatterns` is deprecated. " *
+            "The signature is now " * 
+            "`AmplitudeAwareOrdinalPatterns{m}(τ::Int = 1, A = 0.5, lt::F=isless_rand)`" * 
+            ", so provide `A` as a positional argument instead. "  * 
+            "In this call, the given keyword `A` is used instead of the positional `A`."
+            A = 0.5;
+            @test_logs (:warn, msg) AmplitudeAwareOrdinalPatterns{3}(τ, A; A)
+
+            # Test that keyword argument is preferred over positional argument
+            o = AmplitudeAwareOrdinalPatterns{3}(2; τ = 5)
+            @test o.τ == 5
+            lt = Base.isless;
+            ltr = ComplexityMeasures.isless_rand
+            o = AmplitudeAwareOrdinalPatterns{3}(2, 0.5, Base.isless; lt = ltr)
+            @test o.encoding.lt == ltr
+            o = AmplitudeAwareOrdinalPatterns{3}(2; τ = 5)
+            @test o.τ == 5
+            o = AmplitudeAwareOrdinalPatterns{3}(2, 0.5; A = 0.9)
+            @test o.A == 0.9
+        end
+    end
 
     for f in (OrdinalPatterns, WeightedOrdinalPatterns, AmplitudeAwareOrdinalPatterns)
         a = f(; m = 3, τ = 2)