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

Resample doesn't resample at given rate #186

Open
dsweber2 opened this issue Oct 30, 2017 · 3 comments · May be fixed by #596
Open

Resample doesn't resample at given rate #186

dsweber2 opened this issue Oct 30, 2017 · 3 comments · May be fixed by #596
Milestone

Comments

@dsweber2
Copy link

For example, simply trying to double the length of a signal doesn't actually work:

x = randn(4)
resample(x,4.0)
resample(x,4//1)

both of which return a vector of length 14. I tried various other lengths for x and got that it down by 2 for every case.

I tried altering the amount of zero padding, but increasing the padding by even 1 led to a length 18 signal. I suspect that something is wrong with the actual resample_filter design, but I'm not knowledgeable enough with the material to fix it on my own at this point.

And while we're at it, not being able to put in an integer as a resampling rate is mildly annoying. Is there any design reason for this?

@dsweber2
Copy link
Author

Another issue-- both this and the matlab version assume that the signal is periodic (or at least continuous at the boundaries), leading to some pretty nasty gibbs phenomina (e.g. plot([x for x=0:.1:10])). Since this is pretty easy to fix by mirroring the signal (using resample([x; flipdim(x,1)],2.0)), I'm surprised that neither matlab nor this DSP code does this. If there's a speed concern, checking the difference between the first and last entry relative to the mass of the whole signal (e.g. (x1]-x[end])/norm(x)>ϵ should be a sufficient check (if their neighbors are too far from the current, simply mirroring isn't going to cut it).

@staticfloat
Copy link
Contributor

I don't think that automatically mirroring the signal is a good idea, as I would want to opt into that if I wanted to actually do that.

However, I agree that allowing the multirate filters to eat some samples isn't good; if I ask for a 2//1 resampling factor, I should get exactly 2x as many samples out. For example, I would expect resample(randn(4), 2//1) to give me the number of output samples as is declared by DSP.outputlength(FIRFilter(h, 2//1), length(x)).

@martinholters
Copy link
Member

In my opinion, for integer rate multipliers $M$, there are two reasonable choices for the output length for an input of length $N$:

  • $M*N$, as suggested here and probably the most intuitive choice
  • $M*(N-1)+1$, only interpolating between given samples, not extrapolating beyond that last sample

The second choice is particularly attractive if the interpolation filter is such that it preserves the input samples (i.e. $h(0)=1, h(kM)=0$).

The current implementation seems to give an output length of $\lceil M*(N - 0.5) \rceil$, which I don't see any explanation for.

For rate multipliers $1/M$ ($M$ integer), the output length is $\lceil N/M \rceil$.

I support switching this to $\lceil N/M \rceil$ for all cases, as that appears to be the most consistent and intuitive choice. (Or maybe round-to-nearest instead of ceil, not sure.)

Would we consider such a change breaking? It does change the output in ways that may break people's code (most probably in cases where they've worked around the current quirks), so probably yes. But it's also unlikely to silently lead to wrong results, so doing this in a major version bump (or minor while pre-v1) seems fair. Or would this require some deprecation/opt-in/whatever gymnastics for one release cycle? Putting this on the milestone so that we remember to at least think about a strategy forward.

@martinholters martinholters added this to the 0.8 milestone Nov 5, 2024
martinholters added a commit that referenced this issue Nov 25, 2024
@martinholters martinholters linked a pull request Nov 25, 2024 that will close this issue
martinholters added a commit that referenced this issue Nov 28, 2024
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 a pull request may close this issue.

3 participants