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

Create WelchConfig object #502

Merged
merged 11 commits into from
Feb 24, 2024
Merged

Conversation

haberdashPI
Copy link
Contributor

@haberdashPI haberdashPI commented May 24, 2023

This allows welch_pgram to use a predefined set of configuration parameters in an object named WelchConfig, rather than passing these configuration values by keyword argument. FFT plans and intermediate buffers are preconfigured in WelchConfig making repeated calls that use the same config object faster.

This also sets MTConfig and WelchConfig to be children of the abstract type AbstractPGramConfig


@codecov-commenter
Copy link

codecov-commenter commented May 24, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (be1a084) 97.56% compared to head (3f356c0) 97.58%.
Report is 6 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #502      +/-   ##
==========================================
+ Coverage   97.56%   97.58%   +0.01%     
==========================================
  Files          18       18              
  Lines        3124     3147      +23     
==========================================
+ Hits         3048     3071      +23     
  Misses         76       76              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

src/periodograms.jl Outdated Show resolved Hide resolved
@haberdashPI haberdashPI requested a review from martinholters May 26, 2023 18:59
@ararslan ararslan requested a review from galenlynch June 6, 2023 17:21
@wheeheee wheeheee mentioned this pull request Feb 8, 2024
@wheeheee wheeheee added this to the 0.8 milestone Feb 8, 2024
@martinholters
Copy link
Member

martinholters commented Feb 9, 2024

Should we introduce

periodogram(s::AbstractVector{T}, config::WelchConfig) where T<:Number = welch_pgram(s, config)
periodogram(signal::AbstractVector, config::MTConfig) = mt_pgram(signal, config)

? I'm not sufficiently familiar with these functions to tell whether this makes sense, but if it doesn't, the common supertype AbstractPGramConfig would be questionable as well.

Copy link
Member

@martinholters martinholters left a comment

Choose a reason for hiding this comment

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

Testing welch_pgram! would be good to get it covered as well.

@wheeheee
Copy link
Member

wheeheee commented Feb 9, 2024

On the topic of abstract supertypes, Filter doesn't seem to be doing anything

@martinholters
Copy link
Member

On the topic of abstract supertypes, Filter doesn't seem to be doing anything

True. I wouldn't mind it being deleted if it bothers you.

src/periodograms.jl Outdated Show resolved Hide resolved
Copy link
Member

@martinholters martinholters left a comment

Choose a reason for hiding this comment

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

Generally LGTM.
The suggestions concerning the default for buffer are just that - suggestions.

As I don't know this code too well, another approval before merge would be welcome.

src/periodograms.jl Outdated Show resolved Hide resolved
src/periodograms.jl Outdated Show resolved Hide resolved
src/periodograms.jl Outdated Show resolved Hide resolved
src/periodograms.jl Outdated Show resolved Hide resolved
buffer kwargs

Co-authored-by: Martin Holters <[email protected]>
@wheeheee
Copy link
Member

With so many configs lying around, would it be good to also add configs for Periodogram and Periodogram2? I mean, not in this PR, of course.

Copy link
Member

@wheeheee wheeheee left a comment

Choose a reason for hiding this comment

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

I'm also not too familiar with this code, but I looked through it and I think it's good for merging.
Also FWIW I ran tests for SignalAnalysis.jl master (which uses welch_pgram) with this PR after fixing up digitalfilter and compat there and they passed.
Merging soon, if there aren't further objections.

@martinholters martinholters merged commit 10a7c1e into JuliaDSP:master Feb 24, 2024
11 checks passed
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.

4 participants