JuliaDynamics/Associations.jl

Typo: Missing =

Closed this issue · 2 comments

Just browsing the package, looks awesome!

I was looking at the actual implementation of GaussianMI and I think there's a typo here:
https://github.com/JuliaDynamics/CausalityTools.jl/blob/f1bc2b768778f2670587c87022f5e314e85eab0c/src/methods/infomeasures/mutualinfo/estimators/GaussianMI.jl#L70-L74

The last line should probably be

mi = -0.5 * sum(log(σᵢ) for σᵢ in σ)

Also there's a small typo in the doc here:
https://github.com/JuliaDynamics/CausalityTools.jl/blob/f1bc2b768778f2670587c87022f5e314e85eab0c/src/methods/infomeasures/mutualinfo/estimators/GaussianMI.jl#L49-L51

Should be \sum{n=1} -> \sum_{n=1}. It shows up weird in the docs.

I can make a PR in a bit when I'm back at my workstation.

Well spotted, @RomeoV! You're right about both typos. If you could contribute a PR, that would be excellent 👍

If you can come up with a good test for the normalized GaussianMI case, that would also be awesome to have (the current test suite didn't pick up on the typo, so a test for it has to be missing). If you don't want to do so in your PR, no problem - I'll just open an issue for it and deal with it later.

I'm not super well versed in the theory, but I can try to come up with a test that compares, say, the MI(normalize=true) of unnormalized data with the MI(normalize=false) of the already normalized data, and then also compare those to the theoretical result for some random samples (with fixed seed). Does that seem reasonable?

EDIT: Moved the discussion to a new issue.