Add permentropy and its tests #20

tkf · 2018-01-27T09:34:45Z

closes #18

Here is a demo: https://gist.github.com/tkf/31cb31564dc4f61c085d013656f1a9e9

Just to be in the same style as the other docstrings that accept timeseries and cite papers

Datseris · 2018-01-27T09:46:55Z

Looking good!!! Before we go into performance discussions, may I suggest a name change to permentropy, to have a more similar feel to genentropy ?

tkf · 2018-01-27T09:47:32Z

Yea, I thought about that too. Working on it.

Datseris

Awesome this is a job well done!

I only had performance improvement comments.

Oh I also have this comment about base, what do you think?

Datseris · 2018-01-27T09:56:21Z

src/dimensions/entropies.jl

+    nonzero = [c for c in count if c != 0]
+
+    p = nonzero ./ sum(nonzero)
+    return - sum(p .* log.(base, p))


Will it not be much faster if one does

scale = sum(c) return -sum( (p/scale)*log(p/scale) for p in c if p != 0)

? This version does no allocations, the original does 3 allocations, once when defining nonzero, once when defining p and once when doing p .* log.(base, p) (this also allocates a new array)

Datseris · 2018-01-27T09:57:40Z

src/dimensions/entropies.jl

+function permentropy(
+        time_series::AbstractArray{T, 1}, order::UInt8,
+        interval::Integer = 1;
+        base=Base.e) where {T}


Having the "base" option is cool of course, but the thing is that all other entropies assume base e. Maybe we should add a keyword to all of them?

(Or just use base-e everywhere, which is my suggestion)

I think base=2 is also very popular. In fact, base=2 is used in the original permutation entropy paper. I'd like to have base option everywhere. Or something like genentropy2.

tkf · 2018-01-27T09:59:15Z

Yea it allocates memory but it executed only once. For long time series I don't think it matters.

perms = map(PermType, permutations(1:order)) likely allocates much more memory and doing some optimization for sum(p log(p)) doesn't buy us match, unless we improve perms. I wouldn't worry about it if I were you.

tkf · 2018-01-27T10:02:12Z

Regarding further (memory/CPU) performance improvement around perms representation, probably using some kind of tree structure would help. Maybe this?:
https://www.sciencedirect.com/science/article/pii/0898122194001782

But I'd say this implementation is OK for some moderate order.

Datseris · 2018-01-27T10:04:53Z

The Julia people are super good on that stuff, if you care about it a lot you can post a question on Discourse and people will immediately help. I am sure there are special structures that can add a lot of performance.

At the moment it is not crucial for me and I can merge this and then open an issue about its performance.

What about the base thing?

tkf · 2018-01-27T10:13:24Z

I commented on base things as the review comment. Personally, I'd like to keep it and have base option everywhere.

Datseris · 2018-01-27T10:16:51Z

I commented on base things as the review comment.

Can't find it! Regardless, you are right. Seems like the best option. I will do the change of including base on this PR, is that okay? I will also add my performance benefit, because even if it is small it still exists and does not reduce code clarity.

tkf · 2018-01-27T10:20:48Z

I just wrote:

I think base=2 is also very popular. In fact, base=2 is used in the original permutation entropy paper. I'd like to have base option everywhere. Or something like genentropy2.

tkf · 2018-01-27T10:21:34Z

Re: performance

I like the saying "If you don't benchmark, you don't care the performance." We are not doing any benchmarks here so talking about it is kind of nonsense.

OK. Not fair. I was the first one to bringing that up. But I knew that computational complexity of the innner most loop of the original code was O(N) where N=order! which was ridiculous. I just switched to searchsortedfirst (which uses the binary search) since that would give us O(log N).

tkf · 2018-01-27T10:23:30Z

Probably setting up benchmarks using PkgBenchmark.jl would help us doing further performance improvement.

tkf

Oops! I haven't submitted my review comments. Here they are:

tkf · 2018-01-27T09:36:41Z

src/dimensions/entropies.jl

+    # To compute `p log(p)` correctly for `p = 0`, we first discard
+    # cases with zero occurrence.  They don't contribute to the final
+    # sum hence to the entropy:
+    nonzero = [c for c in count if c != 0]


@Datseris You commented that this is not necessary but I think you need this. Please see my inline comment above.

tkf · 2018-01-27T09:39:28Z

src/dimensions/entropies.jl

+
+    for t in 1:length(time_series) - interval * order + 1
+        sample = @view time_series[t:interval:t + interval * (order - 1)]
+        i = searchsortedfirst(perms, PermType(sortperm(sample)))


Regarding the performance consideration I worried in #18, I think searchsortedfirst is much better than brute-force match. It can be improved, but I think this is a good starting point. See also the comment on PermType above.

tkf · 2018-01-27T09:49:49Z

src/dimensions/entropies.jl

+
+## References
+
+[1] : Bandt, C., & Pompe, B., Phys. Rev. Lett. **88** (17), pp 174102 (2002)


Really? Don't you want to click the doi to get to the paper on browser? (OK, probably consistency matters as well so probably better to do it at once.)

Daaaaamn how did I not consider to that everywhere!

tkf · 2018-01-27T10:07:15Z

src/dimensions/entropies.jl

+function permentropy(
+        time_series::AbstractArray{T, 1}, order::UInt8,
+        interval::Integer = 1;
+        base=Base.e) where {T}


I think base=2 is also very popular. In fact, base=2 is used in the original permutation entropy paper. I'd like to have base option everywhere. Or something like genentropy2.

Datseris · 2018-01-27T10:25:35Z

Probably setting up benchmarks using PkgBenchmark.jl would help us doing further performance improvement.

Yeah sounds like an excellent plan. So far I have developed myself and benchmarked on the same computer with e.g. @btime but now is the time for upgrade.

Edit: #21

Datseris · 2018-01-27T10:44:07Z

Alright, I added base to genentropy. Assuming I did not have 10s of typos and tests pass, this is merged as-is. A separate issue will be created for its performance.

tkf · 2018-01-27T11:01:49Z

Thanks!

BTW, it looks like Travis is failing

Datseris · 2018-01-27T12:29:16Z

merged!

tkf · 2018-01-27T12:44:49Z

Thanks!

Datseris · 2018-01-27T12:45:10Z

thank you!

Add permutation_entropy and its tests

f13a0ad

closes JuliaDynamics#18

tkf force-pushed the permutation_entropy branch from 87310f2 to f13a0ad Compare January 27, 2018 09:42

tkf mentioned this pull request Jan 27, 2018

Permutation entropy #18

Closed

doc string stype change

317ab66

Just to be in the same style as the other docstrings that accept timeseries and cite papers

Datseris self-requested a review January 27, 2018 09:47

Rename: permutation_entropy -> permentropy

a2bd48c

Datseris reviewed Jan 27, 2018

View reviewed changes

type when changing to x

0970e1f

Datseris approved these changes Jan 27, 2018

View reviewed changes

tkf commented Jan 27, 2018

View reviewed changes

Datseris added 2 commits January 27, 2018 11:27

added DOI link!

eed1959

added base to genentropy as well

ad2dc4c

tkf changed the title ~~Add permutation_entropy and its tests~~ Add permentropy and its tests Jan 27, 2018

Datseris added 2 commits January 27, 2018 12:21

relax BK test

e2a4ef3

relax stroboscopic duffing dimension

a8b8abf

Datseris merged commit 6dc5155 into JuliaDynamics:master Jan 27, 2018

Datseris mentioned this pull request Jan 27, 2018

Increase performance of permentropy #22

Closed

Datseris mentioned this pull request Dec 4, 2019

Questions regarding permetropy #95

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add permentropy and its tests #20

Add permentropy and its tests #20

tkf commented Jan 27, 2018 •

edited by Datseris

Datseris commented Jan 27, 2018

tkf commented Jan 27, 2018

Datseris left a comment

Datseris Jan 27, 2018

Datseris Jan 27, 2018

tkf Jan 27, 2018

tkf commented Jan 27, 2018

tkf commented Jan 27, 2018

Datseris commented Jan 27, 2018

tkf commented Jan 27, 2018

Datseris commented Jan 27, 2018

tkf commented Jan 27, 2018

tkf commented Jan 27, 2018

tkf commented Jan 27, 2018

tkf left a comment

tkf Jan 27, 2018

tkf Jan 27, 2018

tkf Jan 27, 2018

Datseris Jan 27, 2018

tkf Jan 27, 2018

Datseris commented Jan 27, 2018 •

edited

Datseris commented Jan 27, 2018

tkf commented Jan 27, 2018

Datseris commented Jan 27, 2018

tkf commented Jan 27, 2018

Datseris commented Jan 27, 2018


		## References

		[1] : Bandt, C., & Pompe, B., Phys. Rev. Lett. 88 (17), pp 174102 (2002)

Add permentropy and its tests #20

Add permentropy and its tests #20

Conversation

tkf commented Jan 27, 2018 • edited by Datseris

Datseris commented Jan 27, 2018

tkf commented Jan 27, 2018

Datseris left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tkf commented Jan 27, 2018

tkf commented Jan 27, 2018

Datseris commented Jan 27, 2018

tkf commented Jan 27, 2018

Datseris commented Jan 27, 2018

tkf commented Jan 27, 2018

tkf commented Jan 27, 2018

tkf commented Jan 27, 2018

tkf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Datseris commented Jan 27, 2018 • edited

Datseris commented Jan 27, 2018

tkf commented Jan 27, 2018

Datseris commented Jan 27, 2018

tkf commented Jan 27, 2018

Datseris commented Jan 27, 2018

tkf commented Jan 27, 2018 •

edited by Datseris

Datseris commented Jan 27, 2018 •

edited