Deprecate filt/filt! with si parameter

[martinholters]

The radical alternative to #599. If you need stateful filtering, use DF2TFilter. After #602, this shouldn't take away any functionality, just restrict the interface to enforce the use of DF2TFilter.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.96%. Comparing base (ade83dd) to head (dbdbb7d).
Report is 1 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #603      +/-   ##
==========================================
+ Coverage   97.91%   97.96%   +0.05%     
==========================================
  Files          19       19              
  Lines        3262     3248      -14     
==========================================
- Hits         3194     3182      -12     
+ Misses         68       66       -2     

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

[wheeheee]

I noticed that the test against Matlab output (filt_check.txt) was dropped. I think that test could have been modified to use DF2TFilter, so was this because of the decision to phase out testing against Matlab samples, or another reason?

[martinholters]

I noticed that the test against Matlab output (filt_check.txt) was dropped. I think that test could have been modified to use DF2TFilter, so was this because of the decision to phase out testing against Matlab samples, or another reason?

Mostly because I was hoping the DF2TFilter is sufficiently tested. Though I didn't check, so that may be false.

But indeed, I was somewhat satisfied to see another of those tests against Matlab samples go. However, if DF2TFilter is indeed undertested, I'll reinstate it. (I'd prefer a test where it's easier to tell what the correct result is than by invoking another filter implementation, but that's for another day...)

[martinholters]

Re-added that test.

[wheeheee]

I do like this a lot more than returning the state. How well would this be received downstream? i.e. does anyone actually pass si to filt?

[wheeheee]

There are some really weird GC allocations from NTuple and Float64 in _filt_fir! when N=15 but I don't see noticeable performance regressions, so whatever.

[wheeheee]

Added another (simpler?) test back. I think 2 tests using an initial state isn't too much.

[martinholters]

How well would this be received downstream? i.e. does anyone actually pass si to filt?

I don't know. I don't think it's particularly useful in its current form (hence #599 and this PR), so widespread use would surprise me. However, I don't want to rush this PR, so that more people get the chance to stumble upon it and raise any objections they might have. That said, I think I'll add deprecations so that code passing si to filt can continue to function and any users get more time to switch to DF2TFilter. (Or complain loud enough that we can add it back during 0.8.x, potentially with #599 reconsidered.)

[wheeheee]

I have a niggling concern about PolynomialRatio, specifically the calls to coefa and coefb which allocate. If DF2TFilter is used to filter relatively short signals, the overhead might be quite large.
One possible solution would be to decouple the FilterCoefficients type parameter from internal storage inside the DF2TFilter type, and store coefs==(coefb(p), coefa(p)) inside DF2TFilter{PolynomialRatio{:z}}. The filter coefficient object could be retrieved by passing the DF2TFilter to, say, FilterCoefficients or another appropriately named function (relatedly, should we provide a public function to retrieve the state?). But would it be worth the added complexity?

[martinholters]

Hm, that's a bit unfortunate indeed. We could also drop the <:FilterCoefficients{:z} from the DF2TFilter type parameter (or relax it) to allow (b,a)::Tuple{Vector,Vector} to be stored as coef. Or we make coefa/coefb non-allocating. The latter would be nice anyway, but is certainly trickier, too. I see two options:

• Implement a small AbstractVector subtype that forwards getindex to the underlying LaurentPolynomial after the necessary index transformation. But that would need some benchmarking how bad this index transformation is in tight loops like filtering.
• Revert PolynomialRatio back to storing the coefficients directly as Vectors. I switched these to LaurentPolynomials a while ago because there was some confusion within DSP.jl how the coefficients had to be interpreted, and by representing them as Polynomials, that was made clear. But I don't think anything critically depends on these being Polynomials.

None of these options strikes me as ideal. But I'm fairly confident there is some way forward, so no reason to block this PR I'd hope.

[wheeheee]

Yup. Thought you wanted to add some deprecations and maybe even more documentation/tests, but I suppose that can go in another PR if needed.

[martinholters]

Deprecations are in the works. Stay tuned.

[wheeheee]

LGTM. I guess for two AbstractVectors the repeat in the deprecated functions basically just copies, but it's good to be clear like this too.

[martinholters]

I guess for two AbstractVectors the repeat in the deprecated functions basically just copies

That should be covered by the more specific deprecations for that case, or am I misunderstanding?

[wheeheee]

Yeah, I meant to say that e.g. lines 7-10 could be omitted. But it's good as is.