ComplexityMeasures.jl icon indicating copy to clipboard operation
ComplexityMeasures.jl copied to clipboard

Triangulation-based estimators

Open kahaaga opened this issue 4 years ago • 11 comments

  • Two new transfer operator estimators: The TransferOperator probabilities estimator now not only accepts RectangularBinning as an argument, but also SimplexPoint and SimplexExact, which uses a triangulated binning of the points.
  • Requires-block. SimplexPoint and SimplexExact uses Simplices.jl, which depends on PyCall, so I used a @requires block for this code to avoid long loading times for the package. That means the user has to run using Simplices after running using Entropies. This is documented in the estimator docstrings.
  • Rename transfermatrix to transitioninfo. Entries of the triangulation-based transfer matrices correspond to simplices, not rectangular boxes. Therefore, to map probabilities of the transfer matrix to partition elements, we need info both about the vertices of the triangulation and which of these vertices form the different simplices. I have therefore created a TransitionInfo return struct that accomodates this information, both for rectangular and triangulation-based binnings.
  • Generator approach: For the transfer operator based probability estimators, use the generator approach as we did in TimeseriesSurrogates.jl. This functionality is not exposed to the user, but used internally. This is implemented because it will be useful in upstream packages, and I don't want to re-implement this in another package when it just as well can live here.

The new estimators and behaviour is documented here.

  • Release Entropies v1.0.0? I don't have any more methods or new functionality to add in the immediate future, and I am very happy with the syntax as is, so I think we can go for v1.0.0. With these added estimators, I have all I need to officially release CausalityTools v1.0 too. What do you think, @Datseris?

kahaaga avatar Feb 20 '21 13:02 kahaaga

Codecov Report

Merging #55 (5559ccb) into master (46d4cf9) will decrease coverage by 10.05%. The diff coverage is 56.62%.

:exclamation: Current head 5559ccb differs from pull request most recent head 89a19f2. Consider uploading reports for the commit 89a19f2 to get more accurate results Impacted file tree graph

@@             Coverage Diff             @@
##           master      #55       +/-   ##
===========================================
- Coverage   74.90%   64.84%   -10.06%     
===========================================
  Files          19       35       +16     
  Lines         522      933      +411     
===========================================
+ Hits          391      605      +214     
- Misses        131      328      +197     
Impacted Files Coverage Δ
src/binning_based/GroupSlices.jl 63.88% <ø> (ø)
src/binning_based/binning_schemes.jl 43.47% <ø> (ø)
...nb_checkpoints/DelaunayTriangulation-checkpoint.jl 0.00% <0.00%> (ø)
...ipes/.ipynb_checkpoints/plot_recipes-checkpoint.jl 0.00% <0.00%> (ø)
...launay_triangulations/plot_recipes/plot_recipes.jl 0.00% <0.00%> (ø)
...pynb_checkpoints/simplex_subsampling-checkpoint.jl 0.00% <0.00%> (ø)
..._based/visitation_frequency/VisitationFrequency.jl 100.00% <ø> (ø)
...ing_based/visitation_frequency/count_box_visits.jl 47.36% <ø> (ø)
...based/visitation_frequency/histogram_estimation.jl 85.41% <ø> (ø)
...tor/triangular/delaunay_triangulations/addnoise.jl 8.82% <8.82%> (ø)
... and 30 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 46d4cf9...89a19f2. Read the comment docs.

codecov[bot] avatar Feb 20 '21 15:02 codecov[bot]

(i'm busy with some other projects at the moment and will look this at the end of the coming week)

Datseris avatar Feb 21 '21 10:02 Datseris

@kahaaga Sorry but we have to discuss this much more before merging. This adds a dependency on PyCall and Python, and I don't think Entropies.jl is should have such a dependency. It also contaminates DynamicalSystems.jl as well with it.

What's the gain of having a python dependency, and what's the need? Julia has advanced quite a bit, are you sure there isn't some Julia implementation of what you need?

(https://github.com/JuliaPolyhedra/QHull.jl is Python)

Datseris avatar Feb 21 '21 14:02 Datseris

What's the gain of having a python dependency, and what's the need? Julia has advanced quite a bit, are you sure there isn't some Julia implementation of what you need?

The dependency on QHull is because of the need for N-dimensional Delaunay triangulations, which are necessary for the triangulation estimators to work. There is no native N-dimensional Julia implementation of that at the moment.

This adds a dependency on PyCall and Python, and I don't think Entropies.jl is should have such a dependency. It also contaminates DynamicalSystems.jl as well with it.

I agree. As a (poor) solution, in the current PR, I've hidden these estimators behind a @requires block, so that PyCall is not loaded until you actually need it (when loading Simplices.jl).

However, this based on old code. I see that there is a relatively new MiniQHull.jl that uses QHull directly. It uses JuliaBinaryWrapper, which wraps QHull directly instead of going through Python/scipy.

I'll see if I can configure Simplices.jl downstream to use MiniQHull.jl instead, so that a dependency on Python here is not necessary. I'll tag you once I have news on that.

kahaaga avatar Feb 21 '21 17:02 kahaaga

@Datseris I had a talk with my collaborators, and we decided that this feature can wait a bit. It turned out to be a lot more work than expected to rewrite stuff to use the QHull library natively.

Our plan is therefore to release CausalityTools 1.0 right away, if that's alright with you.

kahaaga avatar Mar 22 '21 15:03 kahaaga

Hi,

Our plan is therefore to release CausalityTools 1.0 right away, if that's alright with you.

Do you need this PR merged to achieve that?

Datseris avatar Mar 22 '21 16:03 Datseris

@kahaaga you haven't requested feedback on TransferEntropy.jl or CausalityTools.jl docs first. Don't you think it would be useful for another pair of eyes to have a look before making the committment to 1.0?

Datseris avatar Mar 22 '21 16:03 Datseris

Looking over the PR, it changes 37 files. It even adds plotting recipes. What's going on here...?

Datseris avatar Mar 22 '21 16:03 Datseris

Do you need this PR merged to achieve that?

No, not needed.

you haven't requested feedback on TransferEntropy.jl or CausalityTools.jl docs first. Don't you think it would be useful for another pair of eyes to have a look before making the committment to 1.0?

We're doing a last round of checks on the docs in our group the next few days. I will tag you in the v1.0 PR in the CausalityTools repo when ready.

It even adds plotting recipes.

This doesn't need to go here. I'll remove the recipes.

Looking over the PR, it changes 37 files.

I'll re-tag you once the QHull dependencies are done in Simplices.jl. Much of the stuff here will then be moved to that package, so less changes.

kahaaga avatar Mar 22 '21 21:03 kahaaga

Okay it all sounds good to me then! Perhaps it will even cleaner to open a brand new PR later down the line when things are ready!

Datseris avatar Mar 23 '21 11:03 Datseris

Perhaps it will even cleaner to open a brand new PR later down the line when things are ready!

Yes, I think that is a good idea. I'll leave this open for now, just so that I can use CI testing while developing, and open a new PR when everything is cleaned up.

kahaaga avatar Mar 23 '21 19:03 kahaaga