sfstoolbox / sfs-matlab

SFS Toolbox for Matlab/Octave
https://sfs-matlab.readthedocs.io
MIT License
97 stars 39 forks source link

use round() instead of ceil() for integer delay #98

Closed VeraE closed 8 years ago

fietew commented 8 years ago

The integer option was meant to be the ZOH implementation, which does not add additional pre-delay to the input. Changing this to round now is actually wrong.

hagenw commented 8 years ago

Why it is wrong?

With ceil() the problem is that positive and negative delay values are handled asymmetrical.

fietew commented 8 years ago

Because you renamed all the fractional delay methods and integer was zoh before. The functionality you are looking for is a zero-th order lagrange filter.

hagenw commented 8 years ago

I renamed it, because I wanted to have a simple and fast integer delay for people who don't care and don't know what ZOH is.

Would you then propose to 1) Change round() back to ceil() or 2) Add a zero-th order Lagrange filter for integer delay and rename integer back to zoh?

I guess that the 2) solution has the disadvantage that the integer delay will be slower than before?

VeraE commented 8 years ago

because I wanted to have a simple and fast integer delay for people who don't care and don't know what ZOH is.

I totally agree with that. I'd rather use ceil() than thinking about if the implemented zero-th order Lagrange filter is exactly the same as round(). I proposed round() because it has a smaller error than ceil() (and I didn't know this was meant to be ZOH). I guess having both ceil() and round() is a bit over the top?

fietew commented 8 years ago

I agree, that things are not easy to understand, but ZOH is essential tool in digital signal processing, as it does not need any signal samples "from the future". The effect of the non-optimal rounding (due to the ceil()) of the fractional delay is quite interesting for current research in SFS. I would suggest the following compromise: Leave the integer option as it is and add zoh as an additional option. I agree, that although the former is redundant with the zero-th order Lagrange interpolator, it is more intuitive for people who do not care/know.

hagenw commented 8 years ago

That sounds reasonable, I tried to implement this in #100. Could you please have a look if everything is fine.

fietew commented 8 years ago

Looks fine