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

DIVSCALAR off by 1 #83

Open
fbarchard opened this issue Aug 10, 2022 · 2 comments
Open

DIVSCALAR off by 1 #83

fbarchard opened this issue Aug 10, 2022 · 2 comments

Comments

@fbarchard
Copy link

fbarchard commented Aug 10, 2022

The current fixed point DIVSCALAR multiplies by SAMP_MAX which is off by 1

#   define DIVSCALAR(x,k) \
    (x) = sround( smul(  x, SAMP_MAX/k ) )

which produces 0.999938965 / k instead of 1 / k. The value is rounded down, so the larger the k value, the higher the error

it should be

#   define DIVSCALAR(x,k) \
    (x) = sround( smul(  x, (1<<(FRACBITS))/k ) )

It is used in bfly functions like this:

C_FIXDIV(*Fout,2); C_FIXDIV(*Fout2,2);

These are all the instances

FIXDIV(c,div) \
FIXDIV( fk , 2 );
FIXDIV( fnkc , 2 );
FIXDIV( *Fout0,5); C_FIXDIV( *Fout1,5); C_FIXDIV( *Fout2,5); C_FIXDIV( *Fout3,5); C_FIXDIV( *Fout4,5);
FIXDIV(*Fout,2); C_FIXDIV(*Fout2,2);
FIXDIV(*Fout,3); C_FIXDIV(Fout[m],3); C_FIXDIV(Fout[m2],3);
FIXDIV(*Fout,4); C_FIXDIV(Fout[m],4); C_FIXDIV(Fout[m2],4); C_FIXDIV(Fout[m3],4);
FIXDIV(fpk,2);
FIXDIV(fpnk,2);
FIXDIV(scratchbuf[q1],p);
FIXDIV(scratch[q1],p);
FIXDIV(st->tmpbuf[0],2);
FIXDIV(tdc,2);

So 4 constants - 2,3,4,5 and p which is a factor, typically a prime number larger than 5.
FIXDIV(*Fout,2); in bfly2 becomes (v * 16387 + 16384) / 32768
which is accurate for values up to 16384, but off by 1 on larger values
e.g. FIXDIV(20000,2) = (20000 × 16383 + 16384) ÷ 32768 = 9999.889648438 which truncates to 9999.
With k = 4
32767/4 = 8191.
e.g. FIXDIV(20000,4) = (20000 × 8191 + 16384) ÷ 32768 = 4999.889648437 which truncates to 4999.
it should be
(20000 × 8192 + 16384) ÷ 32768 = 5000.5 which truncates to 5000.

@fbarchard
Copy link
Author

fbarchard commented Aug 22, 2022

To work with FIXED_POINT==32 as well as FIXED_POINT==16 this is the change

==== kissfft/_kiss_fft_guts.h#1 - kissfft/_kiss_fft_guts.h ====
74c74
< 	(x) = sround( smul(  x, SAMP_MAX/k ) )
---
> 	(x) = sround( smul(  x, (SAMPPROD)(1u<<FRACBITS)/k ) )

@JulienMaille
Copy link
Contributor

What about submitting a PR?

fbarchard added a commit to fbarchard/kissfft that referenced this issue Sep 19, 2022
Fix for DIVSCALAR off by 1 (mborgerding#83)
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

No branches or pull requests

2 participants