WIP/RFC: Fix output length of resample
#596
Conversation
| @test length(resample(sin.(1:1:35546), 1/55.55)) == 640 | ||
| @test length(resample(randn(1822), 0.9802414928649835)) == 1786 | ||
| @test length(resample(1:16_367_000*2, 10_000_000/16_367_000)) == 20_000_000 | ||
| @test resample(zeros(1000), 0.012) == zeros(12) |
There was a problem hiding this comment.
These lengths now all match expectations:
julia> 35546 / 55.55
639.8919891989199
julia> 1822 * 0.9802414928649835
1786.0
julia> 16_367_000*2 * 10_000_000/16_367_000
2.0e7
julia> 1000 * 0.012
12.0
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #596 +/- ##
=======================================
Coverage 97.90% 97.90%
=======================================
Files 19 19
Lines 3248 3252 +4
=======================================
+ Hits 3180 3184 +4
Misses 68 68 ☔ View full report in Codecov by Sentry. |
|
After some simplification, I think there is a way to make sure that the output length matches. Is there an obstruction to generating |
For
Not sure what you're trying to say here. I guess we could consider changing the state from I tried to avoid putting too many changes in this PR for sake of reviewability, and rewriting the inner workings of |
|
Yeah, that was what I meant. Didn't consider the numerical error...if only there was a better way to calculate |
|
One more thing I'm not 100% sure about: Currently, the target output length is |
| kernel.ϕAccumulator = mod(kernel.ϕAccumulator-1, kernel.Nϕ) + 1 | ||
| if kernel.ϕAccumulator >= kernel.Nϕ | ||
| Δx, kernel.ϕAccumulator = divrem(kernel.ϕAccumulator, kernel.Nϕ) | ||
| kernel.xIdx += round(Int, Δx) |
There was a problem hiding this comment.
| kernel.xIdx += round(Int, Δx) | |
| kernel.xIdx += Int(Δx) |
just to make it explicit that this should not actually round, but just be a type conversion?
| kernel.ϕIdx = floor(Int, kernel.ϕAccumulator) | ||
| kernel.α = kernel.ϕAccumulator - kernel.ϕIdx | ||
| kernel.α, foffset = modf(kernel.ϕAccumulator) | ||
| kernel.ϕIdx = 1 + round(Int, foffset) |
There was a problem hiding this comment.
| kernel.ϕIdx = 1 + round(Int, foffset) | |
| kernel.ϕIdx = 1 + Int(foffset) |
likewise
This may still give results that are inconsistent with how many samples `filt!` produces as the latter accumulates time-deltas (modulo Nϕ) which may be numerically different. However, `outputlength(H, length(x)) == filt!(y, H, x)` should hold more often this way. Also adjust `inputlength` accordingly.
martinholters
force-pushed
the
mh/fix-resample
branch
from
60f2ec7
to
3573b6e
Compare
This comprises a few changes that can be considered in on their own, but later ones depend on earlier ones for proper results. In commit order:
outputlength(::FIRRational, inputlength)is fixed to correctly predict the number of valid samples produced byfilt!. (For context:filt!expects a sufficiently large output buffer and returns the number of samples actually written.)outputlength(::FIRArbitrary, inputlength)is improved to do this correctly more often. Unfortunately, it can still be off-by-one sometimes, which seems almost inevitable. The reason is -- somewhat simplified -- thatfilt!increases the time incrementally, which may be numerically different from considering the whole time-span at once. More complicated in reality, but conceptually: Starting at sample 1 and going in steps of 0.1, how many steps do you need to reach sample 2? Ten, because10 * 0.1 == 1? No, one more, because0.1 + 0.1 + 0.1 + 0.1 + 0.1 + 0.1 + 0.1 + 0.1 + 0.1 + 0.1 < 1. Now, to be sure to haveoutputlengthgive the correct result, I think one would have to replicate the same iterative index/phase computations, which seems excessive.inputlength, specifically forRoundUpandRoundDown(where the latter is the old behavior and still the default). WithRoundDown,inputlengthreturns the largest input length such that the actual output length will be at most the given one, i.e.outputlength(H, inputlength(H, yL)) <= yL < outputlength(H, inputlength(H, yL)+1). OTOH, withRoundUp,inputlengthreturns the shortest input length such that the actual output length will be at least the given one, i.e.outputlength(H, inputlength(H, yL, RoundUp)-1) < yL <= outputlength(H, inputlength(H, yL, RoundUp)).resample, useinputlengthinRoundUpmode to pad the input to the minimum length to get at leastceil(Int, length(x)*rate)output samples, then trim the output to the required size as necessary.Some points I'd especially appreciate feedback on:
@asserts or should that be more defensive?RoundingModeAPI forinputlengthappropriate? OnlyRoundUpandRoundDownreally make sense there, I guess, so maybe not? (Or should we just drop theRoundDownbehavior which might not be needed at all?)Fixes #186.