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

Simplify SurrogateTest #337

Open
Datseris opened this issue Sep 25, 2023 · 5 comments
Open

Simplify SurrogateTest #337

Datseris opened this issue Sep 25, 2023 · 5 comments
Labels

Comments

@Datseris
Copy link
Member

Datseris commented Sep 25, 2023

I see some problems with SurrogateTest.

First, we have the same name in TimeseriesSurrogates.jl. Will we have a conflict of names? Perhaps name this as SurrogateAssocaitionTest? Or, better yet, do we need a new type at all? Why can't we use the existing type from TimeseiesSurrogates.jl as is?

Second, is there a reaspon for SurrogateTest to accept est as input? A user should create and provide an anonymous function with the est. If user wants to use measure with est, then user creates f = (x, y) -> measure(est, x, y) and provides f as first argument. We should demonstrate this in the docstring of SurrogateTest.

Third, I am not sure it is useful to duplicate the table that compares association measures in the docstring. Especially if we remove the redundant est then the table offers no extra information versus the one present in the assocation measures page. EDIT: Okay, I am partly wrong. One needs to say which measures are compatible. But we shouldn't duplicate info on what the measures can do or not w.r.t. pairwise or conditional.

@Datseris Datseris added documentation design-discussion Decisions must be made labels Sep 25, 2023
@Datseris
Copy link
Member Author

Actually, I am not sure how the conditional data are handled. If a user provides an anonymous function, how does one handle the "conditional data" possibility...?

@Datseris
Copy link
Member Author

I guess that's why est is there? But maybe there is a way for est to not be there...

@Datseris
Copy link
Member Author

The more I read the docstring the less understand what est is supposed to be... Why doesn't each instance of AssociationMeasure has an "estimator" as an input? This would simplify things

@haagadev-kristian
Copy link

Hey, @Datseris! These are all good questions.

I'm in the process of rethinking the estimator-measure design here (a necessary consequence of JuliaDynamics/ComplexityMeasures.jl#316). This will be a breaking change.

Perhaps name this as SurrogateAssocaitionTest? Or, better yet, do we need a new type at all? Why can't we use the existing type from TimeseriesSurrogates.jl as is?

I'm also looking into possible designs that allow us to re-use it, but I haven't landed on anything yet. I'm exploring some options as part of the changes implemented here due to JuliaDynamics/ComplexityMeasures.jl#316.

Why doesn't each instance of AssociationMeasure has an "estimator" as an input? This would simplify things

That is one possible solution that I'm looking into. However, it needs to be determined whether this should be an API requirement, or if we should treat it more loosely.

I can't really say anything definitive here before I've finished up the migration to the API in JuliaDynamics/ComplexityMeasures.jl#316, though. I'm super busy at the moment, but will try to finish this up as soon as possible!

@haagadev-kristian
Copy link

I guess that's why est is there? But maybe there is a way for est to not be there...

Yes, that would be optimal. We then have to make sure that this is meaning for all implemented (and future) association measures, not just the information theoretic ones.

kahaaga added a commit that referenced this issue Oct 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants