-
Notifications
You must be signed in to change notification settings - Fork 14
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Separate OutcomeSpace
and ProbabilitiesEstimator
#285
Conversation
Fixes a bug too, where repeated outcomes were returned by `probabilities_and_outcomes`. This wasn't caught before, because the tests were wrong too.
OutcomeSpace
and ProbabilitiesEstimator
Codecov Report
@@ Coverage Diff @@
## main #285 +/- ##
==========================================
+ Coverage 85.88% 86.40% +0.52%
==========================================
Files 57 71 +14
Lines 1438 1751 +313
==========================================
+ Hits 1235 1513 +278
- Misses 203 238 +35
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
It will take me a few days to finalize the remaining estimators I have for this PR, so if you have any feedback @Datseris, please give it, and I will try to incorporate it in the meanwhile. |
Hi @kahaaga , thanks a lot for the PR. Thanks for taking the time to write an extensive PR description that outlines and motivates major changes. I can't stress enough how extremely helpful this description is to review the PR. I went through the changes and the new docs. I have not went through the source code yet, will do only when the PR is final. I think this is incredible and I accept all changes (minor comments only). When merged, I will make a high level tutorial just like I have done for Agents.jl and DynamicalSystems.jl. This library is now so extensive that it really needs such a tutorial.
|
in general please be careful with symbols. I see eg in |
I'll address when I have more time, but for now:
Agreed.
Yes, agreed.
function probabilities_and_outcomes(est::ProbabilitiesEstimator{<:OutcomeSpace}, x)
cts, outs = counts_and_outcomes(est.outcomespace, x)
return Probabilities(cts), out
end Currently, the function is explicitly implemented for each outcome space, but we can make it generic. I'll add this to the TODO-list.
Outcome spaces that don't have counts simply don't implement # MLE estimation on "pseudo-counts"
function probabilities_and_outcomes(est::WaveletOverlap, x)
x isa AbstractVector{<:Real} || error("`WaveletOverlap` only works for timeseries input!")
freqs = time_scale_density(x, est.wl) # relative energies ("pseudo-counts")
return Probabilities(freqs), 1:length(freqs)
end This way, we just consider all "relative counts"/"relative energies"/"relative whatever" as pre-computed probabilities (at least after normalization). What it means in practice is that count-based outcome spaces will work with any |
let me tackle the information change in the same PR as the high level tutorial (after this is merged). |
So I keep the code as is for now on a macro-level, and we deal with #284 after? It is worth noting that |
yes/ |
In the literature, this procedure is almost exclusively referred to as the "maximum likelihood" estimator. Maybe
Edit: maybe |
Hm, I am not sure I like I think I like |
Yes, so the issue is that maximum likelihood estimation is a generic principle that can be applied to many things. So there is a maximum likelihood estimator of entropy, and there is a maximum likelihood estimator of probabilities, and for many other quantities. So in principle, we could use the same name at both levels (this would work, because the type parameterization is generic), but I think that would be confusing from an educational perspective.
I'm not totally happy with |
i don't know what you mean by "pre-normalized". The quantities are just normalized when turned into probabilities. What does the "pre" convey? |
If we made a generic |
833cb06
to
4f272ef
Compare
I don't know where the functions |
okay, turns out source code of |
Forgot to mention: I found what I believe to be a bug in the The following point should be added to the changelog:
|
Can we just remove |
This ties in to my previous comment - we should just delete |
ok done. |
I have to go now but I ]ll be back in 3-4 hours. Test passing is what remains to be done. |
I'll try to fix the remaining tests in the meanwhile. If tests pass when you're back, feel free to merge. I will likely be unavailable until Sunday evening CET. |
The remaining tests fail due to The reason is the definition I think this is why I went with the I actually have a paper I'm working on where I modify these outcome spaces such that they can be counted, but it isn't included here at the moment. Either we need an additional abstract type to dispatch on, or we need another solution. |
well the simplest solution is to explicitly extend the function |
Perfect! I though you removed |
This now looks good from my side. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh man, what a change. but this is awesome! WIll contribute tutorial later tonight!!!!
This PR supersedes #279. Due to type renaming and folder structure changes, the diff is huge. The main changes are summarised below. I may have forgotten some things in this list. To be updated.
This is still WIP, but the API changes can be reviewed. Changes are best viewed in the updated docs.
entropy
without an estimator. #283Probabilities
too #280Changes
I've been experimenting with a few different solutions to #280 and #237. This PR provides a combined solution to both by redesigning the probabilities-based code. Changes are best understood by a few examples:
Examples (comparing old and new API)
Probabilities can now be estimated either 1) using an
OutcomeSpace
directly (which usesMLE
estimation by default, and is equivalent to how probabilities were estimated before), or 2) using a dedicatedProbabilitiesEstimator
, which computes probabilities in some fancy way from raw counts. The probabilities estimator always takes theOutcomeSpace
as the first input.This also works with
information
and friends. We can now arbitrarily combine anyDiscreteInfoEstimator
with anyProbabilitiesEstimator
, which enables a bunch of new estimators, many of which have not been seen in the literature before.Naming:
Codebase:
ProbabilitiesEstimator
becomesOutcomeSpace
. TheOutcomeSpace
defines the possible outcomes, and defines how encoding/discretization of input data is done.ProbabilitiesEstimator
still exists, but has the role of converting outcome counts (or pseudo-counts, in the case of e.g.WaveletOverlap
,PowerSpectrum
orTransferOperator
) to probabilities. The basic estimator isMLE
(maximum likelihood estimator), which estimates probabilities using relative counts. We also now have theBayes
andShrinkage
probabilities estimators, which through parameterisation covers a lot of literature estimators.The transition is done seamlessly by introducing the functions
counts_and_outcomes
/allcounts_and_outcomes
, which are completely analogous toprobabilities_and_outcomes
/allprobabilities_and_outcomes
, except they return raw counts instead of probabilities.This transition is necessary because I realized that probabilities are not the fundamental quantity we though. It is actually the counts of outcomes that are the fundamental quantity, because these define how probabilities are estimated, using some
ProbabilitiesEstimator
. Not allOutcomeSpace
s are counting-compatible (e.g.WaveletOverlap
), but in this implementation we can still use theseOutcomeSpaces
directly with the naiveMLE
probabilities. The justification is that they still return some form of "normalized relative counts", so it fits with theMLE
estimator, even though they are not strictly counts. This avoids breaking any code, and the design choice is documented.The separation into
OutcomeSpace
andProbabilitiesEstimator
is backwards-compatible, because using anOutcomeSpace
directly is equivalent to using theMLE
estimator.Documentation
TODO:
probabilities
/probabilities_and_outcomes
generic. There's no need to specialize methods unless something unique happens for a particularOutcomeSpace
.Renyi
discrete estimatorProbabilitiesEstimator
(the Good-Turing correction)