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

Complexity API #134

Merged
merged 6 commits into from
Oct 18, 2022
Merged

Complexity API #134

merged 6 commits into from
Oct 18, 2022

Conversation

kahaaga
Copy link
Member

@kahaaga kahaaga commented Oct 18, 2022

New complexity API, as agreed on in #133.

As soon as this is merged, I'll bring the non-merged complexity measure PRs up to speed, and they will then also be ready for review.

@codecov
Copy link

codecov bot commented Oct 18, 2022

Codecov Report

Merging #134 (7a793bf) into main (edce107) will decrease coverage by 0.08%.
The diff coverage is 84.21%.

@@            Coverage Diff             @@
##             main     #134      +/-   ##
==========================================
- Coverage   78.27%   78.18%   -0.09%     
==========================================
  Files          30       32       +2     
  Lines         718      729      +11     
==========================================
+ Hits          562      570       +8     
- Misses        156      159       +3     
Impacted Files Coverage Δ
src/complexity.jl 0.00% <0.00%> (ø)
src/symbolization/symbolize.jl 0.00% <0.00%> (ø)
.../complexity_measures/reverse_dispersion_entropy.jl 100.00% <100.00%> (ø)
src/symbolization/GaussianSymbolization.jl 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@kahaaga kahaaga requested a review from Datseris October 18, 2022 13:05
@kahaaga kahaaga mentioned this pull request Oct 18, 2022
3 tasks
Copy link
Member

@Datseris Datseris left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

overall ok, only some debate regarding "normalizing".

@@ -1,8 +1,13 @@
# [Complexity measures](@id complexity_measures)
# [Complexity API](@id complexity_measures)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# [Complexity API](@id complexity_measures)
# [Complexity measures API](@id complexity_measures)

Complexity is the name of a scientific field (the study of complex systems).

"""
ComplexityMeasure

Abstract type for (entropy-like) complexity measures.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Abstract type for (entropy-like) complexity measures.
Supertype for (entropy-like) complexity measures.

(that's how its typically called, see e.g. AbstractDict)

following measures:

- [`ReverseDispersion`](@ref).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shall we add the standard 'see Input data for how Entropies.jl expects input types' here as well? Like in probabilities.

Comment on lines 26 to 32
Estimate the normalized complexity measure `c` for input data `x`, where `c` can
can be any of the following measures:

- [`ReverseDispersion`](@ref).

The potential range `[a, b]` of the output value depends on `c`. See the documentation
strings for the individual measures to get the normalized ranges.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should avoid duplicate information here. We shouldn't list the available measures. Instead, cross reference complexity docstring.

I also am not sure how much of value is the entire last sentence, and I would remove it.

But, what is even the normalized complexity mesure? In entropy this is clearly defined: the value of the entropy divided by the maximum possible value for the given estimator. Isn't it the same here? If so, maybe just say this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should avoid duplicate information here. We shouldn't list the available measures. Instead, cross reference complexity docstring.

That makes sense.

But, what is even the normalized complexity mesure? In entropy this is clearly defined: the value of the entropy divided by the maximum possible value for the given estimator. Isn't it the same here? If so, maybe just say this?

Given that there are many different concepts that fall under the umbrella "complexity measure", there's no way - like for entropy - to know how one would like to normalize it. It would be method-specific.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my latest commit for a suggestion on an improve docstrings.

@kahaaga
Copy link
Member Author

kahaaga commented Oct 18, 2022

overall ok, only some debate regarding "normalizing".

@Datseris I've addressed your comments in the latest commit. Feel free to merge if you're happy with it.

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