Skip to content
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

Triangulation-based estimators #55

Open
wants to merge 30 commits into
base: main
Choose a base branch
from
Open

Triangulation-based estimators #55

wants to merge 30 commits into from

Conversation

kahaaga
Copy link
Member

@kahaaga kahaaga commented Feb 20, 2021

  • 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?

@codecov
Copy link

codecov bot commented Feb 20, 2021

Codecov Report

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

❗ 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.

@Datseris Datseris self-requested a review February 21, 2021 10:16
@Datseris
Copy link
Member

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

@kahaaga kahaaga mentioned this pull request Feb 21, 2021
3 tasks
@Datseris
Copy link
Member

Datseris commented Feb 21, 2021

@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)

@kahaaga
Copy link
Member Author

kahaaga commented Feb 21, 2021

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
Copy link
Member Author

kahaaga commented Mar 22, 2021

@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.

@Datseris
Copy link
Member

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
Copy link
Member

@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
Copy link
Member

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

@kahaaga
Copy link
Member Author

kahaaga commented Mar 22, 2021

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.

@Datseris
Copy link
Member

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!

@kahaaga
Copy link
Member Author

kahaaga commented Mar 23, 2021

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants