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

Fixed interpolation rounding for BC4 #16

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

RunDevelopment
Copy link

@RunDevelopment RunDevelopment commented Aug 1, 2024

Fixes #12

Hi again! Although the discussion around #12 seems to have died down a bit, I decided to make a little PR to fix this issue by fixing the rounding.

Why is the old implementation incorrect?

Integer division truncates the result. For non-negative numbers, this is equivalent to rounding towards zero (floor). To achieve rounded integer division for a divided by b, the old trick is (a + b/2) / b (where / is the truncating division C uses).

This works because for any $a \in N$ and $b \in N, b >0$, the following is true:

$$ round(a/b) = \lfloor a/b+0.5 \rfloor = \lfloor (a + b/2) / b \rfloor = \lfloor (a + \lfloor b/2 \rfloor) / b \rfloor $$

(The last equality is true, because $\lfloor (a + \epsilon) / b \rfloor = \lfloor a / b \rfloor$ for any $\epsilon \in R, 0 \le \epsilon < 1$.)

So if we want to have rounded division by 5 and 7, we need to add 2 and 3 respectively.

The current implementation of (a + 1) / 7 is equivalent to $\lfloor a/7+0.1428572 \rfloor$ or $round(a/7 -0.3571428)$. This makes absolutely no sense mathematically. I think what happened here is that the author knew the trick for rounded division by 2 and 3 ((a + 1) / 2 and (a + 1) / 3 respectively) and incorrectly generalized it to other dividends.

What is the correct behavior?

I strongly believe that rounded division is the correct behavior for the reasons I explained in #15 for BC1. To summarize: I think that the result should be the same as if we were doing everything with floating-point numbers and then converting to integers (via rounding) at the very end. With my suggested fix from #15 being merged already, this new behavior for BC4 (and all that use it internally) would be consistent with BC1.

What did I change?

I changed the + 1 to the correct constants for rounded division by 5 and 7 to fix the rounding. This should come at no additional performance cost, although I haven't benchmarked it.


This comment from #12 suggested using floor/truncated division. I believe that this behavior is incorrect, but I'm in no way opposed to enabling this behavior via a compile-time option, allowing users to choose. (Although I think rounded division should be the default.) The goal of this PR is simply to fix the current incorrectly implemented rounded division.

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.

Interpolation errors in bcdec__smooth_alpha_block
1 participant