Buffer array resample and reference_data
#623
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #623 +/- ##
==========================================
+ Coverage 97.97% 98.13% +0.16%
==========================================
Files 19 19
Lines 3254 3277 +23
==========================================
+ Hits 3188 3216 +28
+ Misses 66 61 -5 ☔ View full report in Codecov by Sentry. |
wheeheee
force-pushed
the
buffer_resample
branch
from
4284e05 to
1c8e69e
Compare
There was a problem hiding this comment.
I generally like DRYing up code by pulling out common code to functions, but those then need a descriptive name to maintain readability. I'm not sure that's always the case here. That aside, this PR increases LOC by 49, 6 of which are a new tests, some are additional usings in the tests (to make those file runable by themselves, I suppose). But that still leaves quite a few additional lines due to NFC code re-organization, where I'm not 100% sure it's a net improvement. Need to let this sink in a bit more.
Meanwhile, some minor remarks.
src/Filters/stream_filt.jl
Outdated
| buffer = allocate_output(self, x) | ||
| bufLen = length(buffer) | ||
| samplesWritten = filt!(buffer, self, x) | ||
| return checked_ff_output(buffer, self, bufLen, samplesWritten) |
There was a problem hiding this comment.
IIUC, filt! doesn't resize buffer, so bufLen here seems redundant?
There was a problem hiding this comment.
bufLen is for checked_ff_output, which also resizes to length samplesWritten if the two aren't equal and the kernel is FIRArbitrary, otherwise it throws.
There was a problem hiding this comment.
But doesn't length(buffer) == bufLen always hold when calling checked_ff_output? Then it shouldn't be necessary to pass it explicitly.
There was a problem hiding this comment.
I see what you mean now, yes.
There was a problem hiding this comment.
Previously I extracted the checks as functions because I wanted to reuse them in _resample! but now there's no need for that, so I'll just inline them back as they were.
src/Filters/stream_filt.jl
Outdated
| if !(Tk <: FIRArbitrary) | ||
| samplesWritten == bufLen || throw(AssertionError("Length of resampled output different from expectation.")) | ||
| end | ||
| samplesWritten >= outLen || throw(AssertionError("Resample output shorter than expected.")) | ||
| length(y) == outLen || resize!(y, outLen) |
There was a problem hiding this comment.
I think this deserves some comments. Let me see whether I get this straight:
bufLen == length(y)is the size of the allocated output.outLenis the desired output lengthrate * length(input), but we may not be able to hit it exactly with the actual processing.samplesWrittenis the number of samples actually written toy. I.e.y[samplesWritten+1:end]contains invalid data (if non-empty).
We ascertain that at least outLen bytes have actually been written and then truncate y to that length.
Is that correct?
test/runtests.jl
Outdated
| @@ -17,6 +17,8 @@ module_tests = [ | |||
| :Unwrap => ["unwrap.jl"] | |||
| ] | |||
|
|
|||
| !(dirname(@__FILE__) in LOAD_PATH) && push!(LOAD_PATH, dirname(@__FILE__)) | |||
There was a problem hiding this comment.
| !(dirname(@__FILE__) in LOAD_PATH) && push!(LOAD_PATH, dirname(@__FILE__)) | |
| !(@__DIR__ in LOAD_PATH) && push!(LOAD_PATH, @__DIR__) |
?
But why is this needed anyway?
There was a problem hiding this comment.
To put FilterTestHelpers in the load path so using FilterTestHelpers doesn't throw. Basically mostly to enable running the file by itself.
Co-authored-by: Martin Holters <[email protected]>
Co-authored-by: Martin Holters <[email protected]>
|
Huh, macOS and 32-bit failures? Weird. |
to `checked_resample_output!`
On top of #621