-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Optimised gaussian blur transform #2347
Optimised gaussian blur transform #2347
Conversation
Reviewer's Guide by SourceryThis pull request optimizes the Gaussian blur transform by utilizing the separable property of Gaussian kernels. It introduces a 1D Gaussian kernel creation function and replaces the 2D convolution with separable convolution, improving the algorithm's time complexity. Additionally, tests were added for the new 1D kernel creation function. Sequence diagram for Gaussian Blur with Separable ConvolutionsequenceDiagram
participant Image
participant GaussianBlurTransform
participant functional.create_gaussian_kernel_1d
participant functional.seperable_convolve
Image->>GaussianBlurTransform: apply(image, transform, **params)
GaussianBlurTransform->>GaussianBlurTransform: get_params_dependent_on_data(params, data)
GaussianBlurTransform->>functional.create_gaussian_kernel_1d: kernel = create_gaussian_kernel_1d(sigma, ksize)
functional.create_gaussian_kernel_1d-->>GaussianBlurTransform: kernel
GaussianBlurTransform->>functional.seperable_convolve: convolve(image, kernel)
functional.seperable_convolve-->>GaussianBlurTransform: blurred_image
GaussianBlurTransform-->>Image: blurred_image
Updated class diagram for Gaussian Blur TransformclassDiagram
class GaussianBlurTransform {
-blur_limit: Tuple[int, int]
-sigma_limit: Tuple[float, float]
+apply(img: np.ndarray, kernel: np.ndarray, **params: Any) : np.ndarray
+get_params_dependent_on_data(params: dict[str, Any], data: dict[str, Any]) : dict[str, float]
+get_transform_init_args_names() : tuple[str, ...]
}
class functional {
+create_gaussian_kernel_1d(sigma: float, ksize: int = 0) : np.ndarray
+seperable_convolve(img: np.ndarray, kernel: np.ndarray) : np.ndarray
}
GaussianBlurTransform --|> functional : uses
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
Hey @vedantdalimkar - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding a test case that compares the output of the original and optimized Gaussian blur implementations to ensure they produce similar results.
- It might be beneficial to include a performance comparison between the original and optimized Gaussian blur implementations in the pull request description.
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟢 Security: all looks good
- 🟡 Testing: 1 issue found
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
@vedantdalimkar Great job. Really nice optimizations. I did not even know that cv2 has separable convolutions. |
@ternaus Just FYI, using cv2.GuassianBlur instead of seperable convolutions was giving similar results (almost equal) in terms of speed. I favored seperable convolutions over cv2.GuassianBlur because it was consistent with the design decision of creating kernels in a similar fashion to PIL. If you want any changes, please let me know. |
I checked. GaussianBlur and Separable give similar performance, with Gaussian being a bit faster. But here we want normalized kernel to match PIL, which prevents from using Gaussian Directly and using separable here was a material 2.5 speedup. Thanks. |
Addresses #2293
Optimised gaussian blur at the algorithm level. Made use of the fact that gaussian kernels are separable, which means that convolution with 2D gaussian kernel can be simplified to 2 convolutions with 1D gaussian kernel which reduces the time complexity of the algorithm.
Time benchmarks -
Old algorithm results -
source_gaussian_blur.txt
Updated algorithm results -
gaussian_blur.txt