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

Refactoring and fixes for the `tutorial` branch #354

Closed kahaaga closed 6 months ago

kahaaga commented 6 months ago

Some necessary changes to get #347 to pass CI and adhere to API.

codecov[bot] commented 6 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

:exclamation: No coverage uploaded for pull request base (tutorial@f010a01). Click here to learn what that means.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## tutorial #354 +/- ## =========================================== Coverage ? 88.98% =========================================== Files ? 79 Lines ? 2206 Branches ? 0 =========================================== Hits ? 1963 Misses ? 243 Partials ? 0 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

kahaaga commented 6 months ago

Fixed missing methods and exports (why were they removed to begin with, @Datseris? I'm thinking specifically of the allcounts etc methods that were generated previously)

If the purpose was to have fewer methods, I'm okay with dropping allprobabilities in favor of just having allprobabilities_and_outcomes. But then all references to the method has to be removed from both the docs, the docstrings, and the tests.

kahaaga commented 6 months ago

Otherwise I'm done here.

Datseris commented 6 months ago
  • Not sure what is the best. Here I just modified the examples in the tutorial to be explicit about using Shannon

I disagree with that. entropy must default to Shannon. To my understanding so far in the wide literature of NLTS 99.9% of cases an entropy is computed, this is the Shannon entropy. Hence this is a sensible default.

For all the rest, okay!

kahaaga commented 6 months ago

I disagree with that. entropy must default to Shannon. To my understanding so far in the wide literature of NLTS 99.9% of cases an entropy is computed, this is the Shannon entropy. Hence this is a sensible default.

Ok, I'm fine with Shannon entropy being the default.

Should I implement it or will you do it? It's just a matter of removing the Shannon instances from the tutorial and changing the else case here

function entropy(args...)
    e = first(args)
    # Check the condition for throwing an error (if false)
    cond = if e isa ProbEstOrOutcomeSpace
        # Shannon is used as default information measure
        true
    elseif e isa InformationMeasure
        # Any subtype of entropy
        e isa Entropy
    elseif e isa InformationMeasureEstimator
        # Estimator is for any subtype of entropy
        e.definition isa Entropy
    else
        false
    end
    cond || throw(ArgumentError("""
        You have used `entropy` without an entropy definition
        ($(typeof(e))). Use `information` instead."""))
    return information(args...)
end
Datseris commented 6 months ago
  • why were they removed to begin with, @Datseris?

That was when counts became the main method and counts_and_outcomes was in the deprecation file. Now of course this is incorrect, as the _and_outcomes is the correct method. Your changes are correct in any case!


Regarding allcounts: yes, neither allcounts nor allprobabilities should exist at all. Both must anwyays explicitly compute all outcomes, hence they have no reason to exist; they can never be optimized to not calculate outcomes. Hence, only the _and_outcomes versions should exist for both. So we should just add allcounts and allproabilities to deprecations file and simply remove all refernece of them from the docs! Let me mnow if you do this in this PR or we do it in the tutorial PR after merging this one in.

kahaaga commented 6 months ago

Regarding allcounts: yes, neither allcounts nor allprobabilities should exist at all. Both must anwyays explicitly compute all outcomes, hence they have no reason to exist; they can never be optimized to not calculate outcomes. Hence, only the _and_outcomes versions should exist for both. So we should just add allcounts and allproabilities to deprecations file and simply remove all refernece of them from the docs! Let me mnow if you do this in this PR or we do it in the tutorial PR after merging this one in.

I can add the deprecations here, no problem.

Datseris commented 6 months ago

Should I implement it or will you do it? It's just a matter of removing the Shannon instances from the tutorial and changing the else case here

Since in my branch Shannon was the default, please restore this behjavior before we merge your PR into mine.

kahaaga commented 6 months ago

Since in my branch Shannon was the default, please restore this behavior before we merge your PR into mine.

I haven't changed the behavior of the code, just the doc examples. Your doc build failed because entropy(p::Probabilities) isn't implemented. It also isn't tested for. But I'll fix both before merging here 👍

Datseris commented 6 months ago

ah sorry. ha my mind was stuck so far in the past where shannon was the default for entropy. that was proably so far in the past before even the information function.

kahaaga commented 6 months ago

ah sorry. ha my mind was stuck so far in the past where shannon was the default for entropy. that was proably so far in the past before even the information function.

Yes, I think this is from 1.X or so :D