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

Simplify bilinear interpolation #132

Closed
samreid opened this issue Mar 26, 2024 · 4 comments
Closed

Simplify bilinear interpolation #132

samreid opened this issue Mar 26, 2024 · 4 comments
Assignees

Comments

@samreid
Copy link
Member

samreid commented Mar 26, 2024

During code review #103 we observed this bilinear interpolation code:

      // Bilinear interpolation: a resampling method that computes a distance-weighted average of the four nearest values,
      // which in this case are the values at the 4 corners of the enclosing rectangle noted above: f00, f10, f01, f11.
      // Common terms have not been factored out so that this matches the form typically shown in references.
      value = ( f00 * ( ( x1 - x ) / ( x1 - x0 ) ) * ( ( y1 - y ) / ( y1 - y0 ) ) ) +
              ( f10 * ( ( x - x0 ) / ( x1 - x0 ) ) * ( ( y1 - y ) / ( y1 - y0 ) ) ) +
              ( f01 * ( ( x1 - x ) / ( x1 - x0 ) ) * ( ( y - y0 ) / ( y1 - y0 ) ) ) +
              ( f11 * ( ( x - x0 ) / ( x1 - x0 ) ) * ( ( y - y0 ) / ( y1 - y0 ) ) );

Please note several quantities are repeated in this expression. Furthermore, note that x1-x0 and y1-y0 are simply the spacing because of the definition of x0,x1,y0 and y1:

      const x0 = columnIndex * this.spacing;
      const x1 = x0 + this.spacing;
      const y0 = rowIndex * this.spacing;
      const y1 = y0 + this.spacing;

Therefore, the bilinear calculation can be written as:

      const dx1 = x1 - x;
      const dy1 = y1 - y;
      const dx0 = x - x0;
      const dy0 = y - y0;
      value = ( f00 * dx1 * dy1 +
                f10 * dx0 * dy1 +
                f01 * dx1 * dy0 +
                f11 * dx0 * dy0 ) / this.spacing / this.spacing;

This simplification makes it easier to grok and see the symmetries.

We observed that dragging the magnet around on screen 1 for 10 seconds ended up calling this method 904544 times. However, on macOS 14.2.1 running chrome Version 123.0.6312.59 (Official Build) (arm64) and firefox 124.0.1 (64-bit), the optimized version did not show faster performance. We did not compare performance of the algorithms on chromebooks or ipads.

// Common terms have not been factored out so that this matches the form typically shown in references.

In our opinion, having a simpler, easier to grok implementation where the symmetries are clear is more valuable than matching the form typically shown in references. We also recommend unit tests to guarantee the behavior, see #131

@pixelzoom
Copy link
Contributor

Please note several quantities are repeated in this expression. Furthermore, note that x1-x0 and y1-y0 are simply the spacing because of the definition of x0,x1,y0 and y1:

Yes I realize that. But as I documented:

// Common terms have not been factored out so that this matches the form typically shown in references.

However, on macOS 14.2.1 running chrome Version 123.0.6312.59 (Official Build) (arm64) and firefox 124.0.1 (64-bit), the optimized version did not show faster performance

And I doubt that any platform's optimization is so bad that this will result in any performance improvement.

@pixelzoom
Copy link
Contributor

pixelzoom commented Mar 27, 2024

I'm resistant to changing this. I'm so used to seeing the form that's currently implemented in the code, and I suspect others might be too, so I'm not at all on board with the "easier to grok" argument. There's no evidence that changing it will result in any performance increase. And I'll need to spend time making the change and regression testing.

I'll give @samreid one more opportunity to sell me on this. Why should I change this?

@pixelzoom pixelzoom assigned samreid and unassigned pixelzoom Mar 27, 2024
@samreid
Copy link
Member Author

samreid commented Mar 28, 2024

I trust your judgment and no changes are necessary here. I wrote this issue because, as an outsider seeing that code for the first time, this part looked overly complex and difficult to understand. My proposal was one idea that was easier for me to understand, to make the code look less overwhelming. Up to you if you want to do anything here.

@samreid samreid assigned pixelzoom and unassigned samreid Mar 28, 2024
@pixelzoom
Copy link
Contributor

Cool. I'm going to leave it as is.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants