JuliaDSP / DSP.jl

Filter design, periodograms, window functions, and other digital signal processing functionality
https://docs.juliadsp.org/dev/
Other
374 stars 108 forks source link

Fix bounds errors when resampling with some arbitrary ratios #539

Closed anowacki closed 4 months ago

anowacki commented 5 months ago

In some cases when filt(::FIRFilter{FIRArbitrary}, x) is called with certain values of x, the buffer is actually one sample too short and a BoundsError is thrown in filt!(buffer, ::FIRFilter{FIRArbitrary}, x). Add one extra sample to the calculated buffer length to catch these exceptional cases, which mostly arise when resampling with particular resampling ratios.

(This code is really a hack and simply works around the deeper problem of ~calculating the correct output buffer length~ writing the correct number of points in filt!; this should instead be addressed properly in a future change.)

Closes #317.

codecov[bot] commented 5 months ago

Codecov Report

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

Comparison is base (9791404) 97.56% compared to head (b790b4e) 97.56%. Report is 2 commits behind head on master.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #539 +/- ## ======================================= Coverage 97.56% 97.56% ======================================= Files 18 18 Lines 3125 3127 +2 ======================================= + Hits 3049 3051 +2 Misses 76 76 ```

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

anowacki commented 5 months ago

The following isn't quite correct:

(This code is really a hack and simply works around the deeper problem of calculating the correct output buffer length; this should instead be addressed properly in a future change.)

The output buffer is the correct length; it is the writing to the buffer in the filt! function which is at fault, as it occasionally writes one point too many in the stream filtering routines. So I have updated the comment and commit message to better reflect that.

ViralBShah commented 5 months ago

@anowacki Would it be helpful for you to have commit access with the repo to help with maintenance? Not a whole lot changes workflow wise (PRs, reviews, etc), but we have more hands to keep things going and to improve the package.

anowacki commented 5 months ago

@ViralBShah I'm not averse to the idea, but I'm really only making PRs for things that I have already sorted out locally to get work done. I would also not consider myself as much of an expert on signal processing as I would like to be to help maintain the repo.

ViralBShah commented 5 months ago

Thanks. Happy to add you later if it makes sense. Just let me know.

ViralBShah commented 4 months ago

Merge?