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
48 stars 11 forks source link

Signature for `Counts` and `Probabilities` docstrings has the wrong type parameter order #370

Closed kahaaga closed 5 months ago

kahaaga commented 5 months ago

The docstrings show

    Probabilities <: Array{N, <: AbstractFloat}
    Probabilities(counts/probs [, outcomes [, outnames]]) → p

`Probabilities` stores an `N`-dimensional array .... 

This is, at least to me, confusing, because abstract arrays in julia always have the element type as the first type parameter and the dimension as the second type parameter. What's the reason for changing this over, @Datseris?

We should probably also think about whether Probabilities(counts/probs [, outcomes [, outnames]]) → p is a good way to indicate that there are two optional positional arguments here. I'd like to call myself a seasoned Julia user, but I haven't seen this notation before, and not having touched the codebase for some time, I had to check the source code to see if I'd done anything weird here before understanding what the syntax meant. Is there any (official?) julia style guide with tips on how to convey stuff like this? I'd rather list a few more method lines instead of nesting it with multiple brackets like we do now.

Datseris commented 5 months ago

What's the reason for changing this over, @Datseris?

nothing, it's a typo.

We should probably also think about whether Probabilities(counts/probs [, outcomes [, outnames]]) → p is a good way to indicate that there are two optional positional arguments here.

Unforutnately, this is the used notation. I have seen it many times in Julia Base. Nevertheless, nothing stops us from having two lines.

Probabilities(counts/probs) 
Probabilities(counts/probs, outcomes [, outnames])

which makes thingfs clear

Datseris commented 5 months ago

counts/probs is weird syntax as it is an operation. IT should be counts_or_probs instead.

kahaaga commented 5 months ago

Closed by https://github.com/JuliaDynamics/ComplexityMeasures.jl/pull/382