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

Add convolve function to numeric extension #347

Merged
merged 1 commit into from Jul 22, 2019
Merged

Conversation

lpranam
Copy link
Member

@lpranam lpranam commented Jul 22, 2019

Description

This function combines both convolve_rows and convolve_cols into a single call for user convenience.

The need for such function came into the sight when @miralshah365 was implementing adaptive_threshold and encountered error due to multiple calls to convolve function with the same source instead of using the result of the first convolution as the source for other.

References

Tasklist

  • Add test case(s) (in #349)
  • Ensure all CI builds pass
  • Review and approve

This function combines both `convolve_rows` and `convolve_cols` into single call for user convenience
@simmplecoder
Copy link
Contributor

@simmplecoder simmplecoder commented Jul 22, 2019

I believe the names are a bit misleading. My first impression was that kernel is allowed to be 2D, but user would have to wait for compilation error or figure out the logic by some other way. Some combination of words "pipe", "2dir" or something like that would be helpful.
It might be me being a bit picky though, looks good otherwise.

Copy link
Member

@mloskot mloskot left a comment

Thanks for this utility.

Formally, it should have been rejected that due to missing tests :-)
but I will add those myself as I'm adding other tests for the convolution implementation

@mloskot mloskot added cat/enhancement ext/numeric labels Jul 22, 2019
@mloskot mloskot added this to the Boost 1.72+ milestone Jul 22, 2019
@mloskot mloskot merged commit 2c4529e into boostorg:develop Jul 22, 2019
9 checks passed
@mloskot
Copy link
Member

@mloskot mloskot commented Jul 22, 2019

@simmplecoder

I believe the names are a bit misleading.

What names?

My first impression was that kernel is allowed to be 2D,

A KernelConcept would have been helpful indeed (update: #348).

@simmplecoder
Copy link
Contributor

@simmplecoder simmplecoder commented Jul 22, 2019

@mloskot
I mean function and kernel names. May be being a bit more specific would help, but I cannot think of a succinct enough name

@mloskot
Copy link
Member

@mloskot mloskot commented Jul 22, 2019

@simmplecoder Right. TBH, I did not find the function names too confusing myself, but I did find the kernel_2d{_fixed} not well documented or intuitive. Well, that's how original authors from Adobe defined it. It's our job to improve it :)

void convolve(const SrcView& src, const Kernel& ker, const DstView& dst,
convolve_boundary_option option=convolve_option_extend_zero) {
convolve_rows<PixelAccum>(src, ker, dst, option);
convolve_cols<PixelAccum>(dst, ker, dst, option);
Copy link
Member

@mloskot mloskot Jul 22, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lpranam This source and destination views overlap in the second call makes me suspicious.
I always tend to use that with intermediate buffer.
On the other hand, the convolve_rows/cols implementation does the buffering.

mloskot added a commit to mloskot/gil that referenced this issue Jul 22, 2019
Minimal test for new function added in boostorg#347 (with identity kernel only).
mloskot added a commit that referenced this issue Jul 22, 2019
Minimal test for new function added in #347 (with identity kernel only).
@lpranam lpranam deleted the convolve branch Jul 23, 2019
@mloskot mloskot changed the title convolve function added Add convolve function to numeric extension Oct 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cat/enhancement ext/numeric
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants