JuliaDynamics / ComplexityMeasures.jl

Estimators for probabilities, entropies, and other complexity measures derived from data in the context of nonlinear dynamics and complex systems
MIT License
56 stars 14 forks source link

Syntax with type parameter `{m}` in `OrdinalPatterns` is not harmonious with the rest of the library #394

Open Datseris opened 8 months ago

Datseris commented 8 months ago

Making the ordinal patterns take m as a type parameter, as opposted to a keyowrd, was likely a mistake. Does it have a perforamnce benefit? If it does, it can only have one if m is given as a constant literal, 3, and not if it is given as a variable m, because in the later case the type can anyways not be inferred.

However, there are so many other types/outcome spaces that do delay embedding, yet m is a keyword. E.g., Dispersion, Cosine, etc. This is not harmonious and can even be confusing to a user ("why does one take m in one way and the other in another way...?").

I think we should revert the ordinal pattern decision and make it be a keyword there as well...

kahaaga commented 8 months ago

This would not be breaking, so should be fine. Either alternative is good with me.

Datseris commented 3 months ago

This is high priority now, because that is the syntax we use in the paper. ANother thing I am taken aback we haven't closed yet. Damn, I should have gone thorugh the issues list before putting the paper online.

kahaaga commented 3 months ago

Are you on it, or should I do it, @Datseris ?

kahaaga commented 3 months ago

Note that we also need to change BubbleSortSwapsEncoding, because it uses the same syntax.

Datseris commented 3 months ago

haven't started yet!

kahaaga commented 3 months ago

Actually, the syntax OrdinalPatternEncoding(m=3) already works - we did that for backwards compatibility when releasing 3.0. We just need to update the docstring.

kahaaga commented 3 months ago

For BubbleSortSwapsEncoding, we do, however, need another constructor

Datseris commented 3 months ago

We just need to update the docstring.

and probably also move the code from deprecated.jl into the main source.

kahaaga commented 3 months ago

Decision to be made: should m, tau, etc be keywords or positional arguments? Currently we have

Dispersion(; c = 5, m = 2, τ = 1, check_unique = true)
 CosineSimilarityBinning(; m::Int, τ::Int, nbins::Int)
 SequentialPairDistances(x::AbstractVector; n = 3, metric = Chebyshev(), m = 3, τ = 1)
 etc...

while (after updating syntax) we have

OrdinalPatterns(m = 3, τ = 1, lt::Function = ComplexityMeasures.isless_rand) # positional
# and the same for the other ordinal pattern outcomes spaces

I like keyword arguments better. Any objections?

Datseris commented 3 months ago

Keyword arguments are fine!