Closed ikottlarz closed 1 year ago
Nice catch, @ikottlarz!
There's no example in this PR, though. Did you forget to commit it?
@kahaaga looks like I did! Added it now!
@kahaaga looks like I did! Added it now!
Thanks! I'll just wait and see how the doc build looks before merging this.
@ikottlarz Could you also increment the patch version?
Merging #262 (45ea8a6) into main (ac38d75) will decrease coverage by
0.12%
. The diff coverage is100.00%
.
@@ Coverage Diff @@
## main #262 +/- ##
==========================================
- Coverage 82.08% 81.97% -0.12%
==========================================
Files 50 50
Lines 1306 1298 -8
==========================================
- Hits 1072 1064 -8
Misses 234 234
Impacted Files | Coverage Δ | |
---|---|---|
src/complexity_measures/statistical_complexity.jl | 100.00% <100.00%> (ø) |
:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more
This was also a mistake from our part. We should have reviewed this PR more carefully, since it was from a first time contributor that doesn't know all the API by heart. Next time I'll be more thorough with the review before merging.
Agreed, @Datseris. Learning opportunity taken.
I'll just wait and see how the doc build looks before merging this.
The documentation example looks great, @ikottlarz, so I'm merging this.
Just to be clear: Am I right to conclude from this PR that there isn't a mistake in the Rosso paper, but that the discrepancies were due to this issue with not considering the empty bins explicitly?
Just to be clear: Am I right to conclude from this PR that there isn't a mistake in the Rosso paper, but that the discrepancies were due to this issue with not considering the empty bins explicitly?
@kahaaga yes, you are right. I believe the reason the k-noise looked the same as in the paper, but the chaotic maps did not, was because the latter have a lot of missing patterns - the k-noise not so much. It's interesting that applying a different metric in combination with the bug lead to a plot that looked very similar to the Rosso paper.
I believe the reason the k-noise looked the same as in the paper, but the chaotic maps did not, was because the latter have a lot of missing patterns - the k-noise not so much.
When you say missing patterns, are you referring to the number of possible outcomes not visited by the orbit? I.e., the chaotic maps skipped quite a few of the possible outcomes, while the k-noise were more evently distributed among the possible outcomes? (edit: so yielding more explicit zeros for the logistic maps, but not for the noise?)
It's interesting that applying a different metric in combination with the bug lead to a plot that looked very similar to the Rosso paper.
Indeed it is! :D Kudos for figuring it out so quickly. It probably saved us headache down the line when trying to reproducing more stuff from the paper(s).
When you say missing patterns, are you referring to the number of possible outcomes not visited by the orbit? I.e., the chaotic maps skipped quite a few of the possible outcomes, while the k-noise were more evently distributed among the possible outcomes? (edit: so yielding more explicit zeros for the logistic maps, but not for the noise?)
Yes.
Hi there,
turns out I made a mistake in the code! This is the bugfix.
What was wrong?
I assumed that
probabilities(x)
would return the full histogram (including empty bins), as that's what happens in my python implementation. Big mistake. This made the tests pass for edge cases and the min/ max curves, because the latter were calculated with artificial histograms that contained all (including empty) bins.I have now fixed this bug and also added the example from the Rosso 2007 paper.