JuliaDynamics / ComplexityMeasures.jl

Estimators for probabilities, entropies, and other complexity measures derived from data in the context of nonlinear dynamics and complex systems
MIT License
48 stars 11 forks source link

Add functionality clarifying method to codify #359

Closed rusandris closed 6 months ago

rusandris commented 6 months ago

Error message clarification of codify discussed here #358

kahaaga commented 6 months ago

This also needs a test. You can use @test_throws and test that an argument error is thrown for that combination of input types.

codecov[bot] commented 6 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (075eddf) 89.05% compared to head (a51dc84) 89.06%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #359 +/- ## ======================================= Coverage 89.05% 89.06% ======================================= Files 79 79 Lines 2221 2223 +2 ======================================= + Hits 1978 1980 +2 Misses 243 243 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

kahaaga commented 6 months ago

Thanks for the PR, @rusandris!

This closes #359.

Datseris commented 6 months ago

Hello, why was this added as an additional method instead of an if clause inside an existing method?

This is rather a-typical in my experience. Shouldn't we simply have had an if-clause that checks if dimension(ssset) == m? This way we also avoid relying on type parameters that are not public API. The type parameters of StateSpaceSet are not public. The function dimension is. Furthermore the method here is specialized on StateSpaceSet rather than the more general AbstractStateSpaceSet.

I think the if clause also makes the code more readable as the caught clause is not in a completely separate part of the code.

kahaaga commented 6 months ago

I think the if clause also makes the code more readable as the caught clause is not in a completely separate part of the code.

Sure, I'll drop a PR that moves this to the existing if-sentence and uses the abstract state space set.

Datseris commented 6 months ago

First of all, thanks a lot @rusandris and @kahaaga for finding the issue and solving it already. I don't want to appear unappreciative with my comment :D I should have said this before raising any concerns!!!!

And thanks @kahaaga for taking care of the quick fix to improve readability!

kahaaga commented 6 months ago

First of all, thanks a lot @rusandris and @kahaaga for finding the issue and solving it already. I don't want to appear unappreciative with my comment :D I should have said this before raising any concerns!!!!

No problem! More input is always good, and I agree with your assessment on the readability.

And thanks @kahaaga for taking care of the quick fix to improve readability!

I now also updated #360 to use dimension instead of the type parameter for the state space set. Feel free to merge when/if tests pass, @Datseris.

rusandris commented 6 months ago

First of all, thanks a lot @rusandris and @kahaaga for finding the issue and solving it already. I don't want to appear unappreciative with my comment :D I should have said this before raising any concerns!!!!

No worries! I think we managed to converge to a better solution. Thanks for your help, much appreciated.