Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix bounds errors when resampling with some arbitrary ratios #539

Merged

Conversation

anowacki
Copy link
Contributor

@anowacki anowacki commented Feb 18, 2024

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.

@anowacki anowacki marked this pull request as draft February 18, 2024 22:50
@anowacki anowacki force-pushed the an/fix-resample-bounds-error branch from 7f85d33 to 268f3be Compare February 18, 2024 22:56
@anowacki anowacki marked this pull request as ready for review February 18, 2024 22:59
Copy link

codecov bot commented Feb 18, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

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
@@           Coverage Diff           @@
##           master     #539   +/-   ##
=======================================
  Coverage   97.56%   97.56%           
=======================================
  Files          18       18           
  Lines        3125     3127    +2     
=======================================
+ Hits         3049     3051    +2     
  Misses         76       76           

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

Copy link
Member

@martinholters martinholters left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an ugly hack, but the situation is certainly better than before...

In some cases when `filt(::FIRFilter{FIRArbitrary}, x)` is called
with certain values of `x`, `filt!(buffer, ::FIRFilter{FIRArbitrary}, x)`
tries to write one sample too many to the buffer and a `BoundsError`
is thrown.  Add one extra sample to the buffer to catch these
exceptional cases.

(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.)

Closes JuliaDSP#317.
@anowacki anowacki force-pushed the an/fix-resample-bounds-error branch from 268f3be to b790b4e Compare February 20, 2024 11:43
@anowacki
Copy link
Contributor Author

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
Copy link
Contributor

@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
Copy link
Contributor Author

@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
Copy link
Contributor

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

@ViralBShah
Copy link
Contributor

Merge?

@martinholters martinholters merged commit 73d3214 into JuliaDSP:master Mar 11, 2024
11 checks passed
@anowacki anowacki deleted the an/fix-resample-bounds-error branch March 11, 2024 09:42
@tknopp
Copy link

tknopp commented Aug 17, 2024

I ran into this issue and found that it is fixed on the main branch. @martinholters would it be possible to trigger a release including this fix?

@tknopp tknopp mentioned this pull request Sep 1, 2024
martinholters pushed a commit that referenced this pull request Sep 13, 2024
In some cases when `filt(::FIRFilter{FIRArbitrary}, x)` is called
with certain values of `x`, `filt!(buffer, ::FIRFilter{FIRArbitrary}, x)`
tries to write one sample too many to the buffer and a `BoundsError`
is thrown.  Add one extra sample to the buffer to catch these
exceptional cases.

(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.)

Closes #317.

(cherry picked from commit 73d3214)
martinholters pushed a commit that referenced this pull request Sep 13, 2024
In some cases when `filt(::FIRFilter{FIRArbitrary}, x)` is called
with certain values of `x`, `filt!(buffer, ::FIRFilter{FIRArbitrary}, x)`
tries to write one sample too many to the buffer and a `BoundsError`
is thrown.  Add one extra sample to the buffer to catch these
exceptional cases.

(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.)

Closes #317.

(cherry picked from commit 73d3214)
martinholters added a commit that referenced this pull request Sep 27, 2024
Backport `filt` fix (#516 and #539) and bump version to 0.7.10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Got "BoundsError" while trying to use resample function.
4 participants