-
Notifications
You must be signed in to change notification settings - Fork 21
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
Implement Box blur fast filter that could approximate gaussian filter #223
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you add benchmarks to it as well?
You mean in crates/kornia-imgproc/benches/bench_filters.rs ? Sure ok |
mod tests { | ||
use super::*; | ||
|
||
#[test] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would some simple numbers test too similar to the other functions to verify that’s doing the right thing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I added 2 tests here: test_box_blur_fast() and test_gaussian_blur(). Both has the same input (0..25) to show that the outputs are not that much different.
I did attempt to use the same input (all 0.0, 9.0 in the middle) as test_fast_horizontal_filter(), if that's how you mean by this comment. The result was a little disappointing as there's a big difference between the outputs of the 2 methods. I figured it's because the test input was odd. It might be fitting for test_fast_horizontal_filter() but not for these. Therefore I went with something more randomized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why should be different? The test you describe should give you a box of ones, right ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really. The test that gives me a box of ones was because the fast_horizontal_filter() was applied twice. But an actual fast_box_blur() test has the filter applied 6 times (or more). The numbers are a lot more spread out.
@johnnv1 any idea why python tests are failing (I believe it’s unrelated to this PR). Shouldn’t we be using the new just commands in https://github.com/kornia/kornia-rs/blob/main/.github/workflows/python_test.yml#L40 |
yeah, seems unrelated, but should be working |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you expand this benchmark and report the numbers so that we know wether this method is really making what’s expected ?
https://github.com/kornia/kornia-rs/blob/main/crates/kornia-imgproc/benches/bench_filters.rs
I highly suggest once you have the benchmark setup that you play around with it and try to do micro optimisations like reusing as much as possible pre-computed variables as I suggested in the review to see how affects in the benchmarks.
mod tests { | ||
use super::*; | ||
|
||
#[test] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why should be different? The test you describe should give you a box of ones, right ?
So regarding performance, box_blur_fast() filter is independent of the kernel size, but you have to apply the fast_horizontal_filter() 6 times over in 1 run. Therefore it would be slower than native Gaussian blur when the kernel size is small. Here's the performance with all of your suggested micro-optimization applied:
Maybe there're some other optimizations I can do? I'm kinda afraid to introduce unsafe Rust to my code but that's something I could try. |
solve #168. The algorithm was derived from this blog post