DSP.jl icon indicating copy to clipboard operation
DSP.jl copied to clipboard

Add frequency estimators by Jacobsen and Quinn

Open mbaz opened this issue 1 year ago • 10 comments

This commit adds two new frequency estimation algorithms:

* `jacobsen`, a very fast algorithm that performs a three-point
  curve fit around the DFT peak. Not super accurate.
* `quinn`, a very accurate iterative algorithm based on ARMA(2,2).
  It requires an initial frequency estimate (which may be
  provided by `jacobsen`).

Both algorithms work with real and complex signals.

mbaz avatar Jun 28 '23 14:06 mbaz

Is this good to merge?

ViralBShah avatar Jan 20 '24 14:01 ViralBShah

Codecov Report

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

Project coverage is 97.63%. Comparing base (10a7c1e) to head (64058c6).

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #503      +/-   ##
==========================================
+ Coverage   97.58%   97.63%   +0.05%     
==========================================
  Files          18       18              
  Lines        3147     3216      +69     
==========================================
+ Hits         3071     3140      +69     
  Misses         76       76              

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

codecov-commenter avatar Jan 20 '24 15:01 codecov-commenter

Added a couple of tests to include the two lines that were missing coverage.

mbaz avatar Jan 20 '24 23:01 mbaz

Perhaps good to wait for a bit for @martinholters to take a look.

ViralBShah avatar Jan 20 '24 23:01 ViralBShah

@martinholters Thank you for the review!

mbaz avatar Jan 23 '24 14:01 mbaz

@mbaz In preparation for a 0.8 release, would it be possible for you to get this PR ready?

ViralBShah avatar Feb 07 '24 14:02 ViralBShah

@ViralBShah I'm still actively working on it. Two things happened: (1) I realized applying Jacobsen to real signals requires a bit of care, and (2) my work load peaked the past two weeks.

My plan at the moment is to have a single Jacobsen estimator based on the DFT. If/when I work out the issues with an RFFT-based estimator, I'll make another PR. How does that sound?

What is the timeline for 0.8? I will try to finish up this PR in the next couple of days.

mbaz avatar Feb 07 '24 19:02 mbaz

Thank you. There is no tight deadline, and we can always add new features in new releases. I am just thinking out aloud, but if we can get as much of the backlog of PRs in as we can in the next 2 weeks or so - that will make for a nice 0.8 release.

@martinholters I would of course defer to you on that.

ViralBShah avatar Feb 07 '24 19:02 ViralBShah

Agree on the 0.8 release. Would be nice to get of bunch of things done for it which are (and some of them have been for while) in an almost-ready state. OTOH, I don't want to release 0.8 too hastily in case we discover something else that requires a breaking change. My gut feeling is that 2 to 4 weeks from now might be a good time.

martinholters avatar Feb 08 '24 08:02 martinholters

@mbaz Just a gentle nudge to see if possible to finish this PR. 0.8 may be in a week or two. But of course, if you are busy, there's always 0.9!

ViralBShah avatar Feb 18 '24 15:02 ViralBShah

The test failure seems to be related to codecov.

mbaz avatar Feb 20 '24 23:02 mbaz

Thank you too, @martinholters -- the code is much better now than it was in the original PR.

mbaz avatar Feb 26 '24 19:02 mbaz