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
53 stars 12 forks source link

Entropy-type specific dispatch no longer possible for differential entropy. #247

Closed kahaaga closed 1 year ago

kahaaga commented 1 year ago

One of the latest re-writes deprecated entropy(e::EntropyDefinition, est::DiffEntropyEstimator, x).

Problem

Because of this deprecation, it is no longer possible upstream (in CausalityTools) to use multiple dispatch to create estimators of higher-level measures using DifferentialEntropyEstimators. Why? The estimators don't have any type parameters to dispatch on that indicate which type of entropy they compute. Therefore, the algorithms don't have any information about whether a given DifferentialEntropyEstimator is meant for Shannon, Renyi or Tsallis, etc.

There are three ways of dealing with this currently:

  1. Disallow all DifferentialEntropyEstimators as input to any higher-level measure, because we can't guarantee that the correct quantity is computed, since there is no way of checking the input estimator for compatibility.
  2. Manually keep track of a list of which estimators in this package are meant for Shannon, Renyi, etc and keep it up to date at all times between packages. This ensures correctness, but is extremely cumbersome and defeats the whole purpose of using a modular structure with the entropies/estimators.
  3. Ignore it.

Currently, I favor option 3. Why? All the current differential entropy estimators in this package estimate Shannon entropy, so you can't go wrong at the moment. However, the second we introduce an estimator here that computes something else than Shannon differential entropy, users of CausalityTools will be able to use incompatible differential entropy estimator to compute any quantity. Then we'd have to version-lock ComplexityMeasures upstream to avoid issues.

Suggested fix

The better option, though, is to just fix this here immediately. I propose to just do introduce the entropy type as a parameter to DifferentialEntropyEstimator.

 abstract type DifferentialEntropyEstimator{E} end # E is an entropy definition

The existing estimators would be modified to

Base.@kwdef struct Goria{E <: Shannon, B} <: NNDiffEntropyEst
    e::E = Shannon()
    k::Int = 1
    w::Int = 0
    base::B = 2
end

And for any new estimator capable of estimating e.g. Renyi, Tsallis and Shannon entropy,

Base.@kwdef struct NewEstimator{E <: Union{Shannon, Renyi, Tsallis}, B} <: NNDiffEntropyEst
    e::E = Tsallis()
    # params...
end

With this small change, it is trivial to use dispatch for ensuring estimator compatibility upstream.

Datseris commented 1 year ago

Why not simply introduce a function

entropy_definition(e::DiffEntropyEstimator) = Shannon()

and when a new estimator comes along, they can extend this function. In your code that for some reason cares about this information, you need to write a 1 loc wrapper

function your_f(est::DiffEntropyEst, x)
your_f(entropy_definition(est), est, x)
end

and you may dispatch as normal in the second version.

This is also non breaking.


Nevertheless, your proposal is also fine with me. To have the first type parameter of each Diff Ent Estimator to be the type of entropy definition. You'd need to write some extra code for estimators that do not have any entropy type (only apply to shannon), as you cannot extract this from input data during construction, but no big deal.

Datseris commented 1 year ago

actually i prefer your solution

kahaaga commented 1 year ago

Nevertheless, your proposal is also fine with me. To have the first type parameter of each Diff Ent Estimator to be the type of entropy definition. You'd need to write some extra code for estimators that do not have any entropy type (only apply to shannon), as you cannot extract this from input data during construction, but no big deal.

As long as we can use the same code for all differential entropy estimators, that will be enough to simplify things upstream. I also vote in favour of letting the first type parameter be the EntropyDefinition. That makes dispatch much cleaner.

actually i prefer your solution

Ok, then I'll submit a PR for that.

Datseris commented 1 year ago

We fixed this right? Close?