Do not restrict period and phase_shift to the same concrete type

[bencottier]

This restricts them to the same concrete type.

julia> Periodic(sin, Week(1), Day(5))
ERROR: MethodError: no method matching Periodic(::typeof(sin), ::Week, ::Day)
Closest candidates are:
  Periodic(::Any, ::T) where T at /Users/bencottier/.julia/packages/Transforms/NdL6j/src/periodic.jl:36
  Periodic(::Any, ::T, ::T) where T at /Users/bencottier/.julia/packages/Transforms/NdL6j/src/periodic.jl:24
Stacktrace:
 [1] top-level scope at REPL[99]:1

It was unintended, only meant to restrict to the same abstract type, i.e. both Real or both Period.

We need to relax it to exactly match FeatureEngineering, but I don't think it affects results, so I'm not sure if it's worth it.

[bencottier]

The idea I have for implementation gets complicated with parametric types and typeofs:

struct Periodic{T <: Union{Real, Period}, U <: T, V <: T} <: Transform
    f::Union{typeof(cos), typeof(sin)}
    period::U
    phase_shift::V

    function Periodic(f, period <: T, phase_shift <: T) where T <: Union{Real, Period}
        period > zero(typeof(period)) || throw(ArgumentError("period must be strictly positive."))
        return new{T, typeof(period), typeof(phase_shift)}(f, period, phase_shift)
    end
end

[glennmoy]

The idea I have for implementation gets complicated with parametric types and typeofs:

you could probably do something like

struct Periodic{P, S} <: Transform where {P <: T, S <: T} where T <: Union{Real, Period}
    f::Union{typeof(cos), typeof(sin)}
    period::P
    phase_shift::S

    function Periodic(f, period::P, phase_shift::S) where {P, S}
        period > zero(P) || throw(ArgumentError("period must be strictly positive."))
        return new{P, S}(f, period, phase_shift)
    end
end

to clean it up a bit better you could define PeriodicType = Union{Real, Period} or something

[bencottier]

you could probably do something like

The problem is that allows you to mix the types (just calling it Periodic2 to avoid clash in my environment):

julia> struct Periodic2{P, S} <: Transform where {P <: T, S <: T} where T <: Union{Real, Period}
           f::Union{typeof(cos), typeof(sin)}
           period::P
           phase_shift::S

           function Periodic2(f, period::P, phase_shift::S) where {P, S}
               period > zero(P) || throw(ArgumentError("period must be strictly positive."))
               return new{P, S}(f, period, phase_shift)
           end
       end

julia> Periodic2(sin, Day(1), 1.0)
Periodic2{Day,Float64}(sin, Day(1), 1.0)

They need to be the same abstract type for the math to work in either case, because e.g.

julia> 2.0 / Day(2)
ERROR: MethodError: no method matching /(::Float64, ::Day)
...

julia> 2.0 - Day(2)
ERROR: MethodError: no method matching -(::Float64, ::Day)
...

and it's not well-defined anyway.

To solve it, we could throw an error in the constructor if (P <: Period && S <: Real) || (P <: Real && S <: Period), but again that's messy.

[bencottier]

c.f. Discourse suggestions

[glennmoy]

c.f. Discourse suggestions

Interesting. another alternative would be to have two inner constructors

You could make this more compact maybe by using @eval and looping over Real and Period.

julia> struct Periodic{P, S}
              f::Union{typeof(cos), typeof(sin)}
              period::P
              phase_shift::S

              function Periodic(f, period::P, phase_shift::S) where {P<:Real, S<:Real}
                  return new{P, S}(f, period, phase_shift)
              end

              function Periodic(f, period::P, phase_shift::S) where {P<:Period, S<:Period}
                  return new{P, S}(f, period, phase_shift)
              end
           end

julia> Periodic(cos, 2, 1.0)
Periodic{Int64,Float64}(cos, 2, 1.0)

julia> Periodic(cos, Day(2), Week(1))
Periodic{Day,Week}(cos, Day(2), Week(1))

julia> Periodic(cos, Day(2), 1.0)
ERROR: MethodError: no method matching Periodic(::typeof(cos), ::Day, ::Float64)
Closest candidates are:
  Periodic(::Any, ::P, ::S) where {P<:Real, S<:Real} at REPL[3]:6
  Periodic(::Any, ::P, ::S) where {P<:Period, S<:Period} at REPL[3]:10
Stacktrace:
 [1] top-level scope at REPL[6]:1

[bencottier]

another alternative would be to have two inner constructors

Hmm that's pretty nice. Only saw this after posting #32

[glennmoy]

Hmm that's pretty nice. Only saw this after posting #32

🤷 6 of 1, half a dozen of the other