-
Notifications
You must be signed in to change notification settings - Fork 57
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 dog_response
; improve doc of gaussian_blur
#261
Conversation
This related to a bugfix: kornia/kornia#3137 |
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.
The PR looks quite good. Can we benchmark this against image-rs or if we find another implementation. To have a sense how’s the behaviour?
I checked their repo, but I did not find the implementation of difference_of_gaussians, only gaussian_blur. |
You could try to implement dog with image-rs (two blurs+diff)? And yes let´s try few and choose the best one |
dog_response
; improve doc of gaussian_blur
Benchmark Result
I think Dog Response/dog_response_row_parallel/32x32 |
Hi @edgarriba, do you have any further suggestions or comments? |
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.
interesting to see that serial is the one with the better trade off, right ?
yes, this is why we need benchmark :) |
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 also proposed in another PR about the possibility to provide the three variants serial/parallel_row/parallel_full with proper docs and hints about performance. What do you think ?
Here's the newest benchmark result |
I think the idea of providing all three variants—serial, parallel_row, and parallel_full—with proper docs and performance hints is definitely interesting. However, looking at the benchmark results, it seems the serial approach either outperforms or comes very close to the parallel implementations across nearly all scenarios. |
alright, then go ahead with the serial implementation |
Does it seem like a good time to merge now? |
Resolve #245