-
Notifications
You must be signed in to change notification settings - Fork 17
Powerspectrum estimator add threshold #442
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
base: main
Are you sure you want to change the base?
Powerspectrum estimator add threshold #442
Conversation
that adds a threshold for "small" amplitudes. See Issue JuliaDynamics#104.
feature. * Changed the default threshold from 5.0 to 0.0 * Renamed y to amp_squared to make it more clear * Vectorized the setting of amplitudes to zero
Datseris
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some simplifying and performance comments
|
Thanks for the PR @elavallee , in addition please increment the minor version (middle number) at Project.toml file by 1 and add an entry about this change in the CHANGELOG.md file! |
Co-authored-by: George Datseris <datseris.george@gmail.com>
Co-authored-by: George Datseris <datseris.george@gmail.com>
…b.com/elavallee/ComplexityMeasures.jl into powerspectrum_estimator_add_threshold
*Updated the minor version number. *Updated the changelog.
|
Hi @Datseris, I appreciate your changes. I've incorporated them. I also updated the minor version number and the CHANGELOG.,md. Thank you. |
Updated version number and added a new feature description for the PowerSpectrum estimator.
Datseris
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm sorry @elavallee , I forgot something in my review. There are no tests for this new feature... Can we please add a test? For example testing that the probabilities for the signal x = cos.(range(0, 2π; length = 10000) .+ 1e-2 .* randn(10000) with some δ = 0.1 are all 0 except from a single probability?
This functionality should be tested. Even though I have no doubt the code is correct as it is, it helps for forwards maintainance if all parts of the code are test covered.
Please review my change to add a threshold to the PowerSpectrum estimator. The optional threshold is named:$\delta$ . I updated the docstring, and I was able to pass all the tests locally. However, I did not add any new tests, which is more challenging. For an example of how to use the new feature, please see the code below.
`