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_2d function to numeric extension #367

Merged
merged 1 commit into from Aug 8, 2019

Conversation

miralshah365
Copy link
Contributor

@miralshah365 miralshah365 commented Aug 5, 2019

Description

convolve function renamed to convolve_1d

for more detail check the reference(s)

References

closes #356

Tasklist

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

@miralshah365 miralshah365 self-assigned this Aug 5, 2019
@mloskot mloskot added cat/feature status/work-in-progress labels Aug 5, 2019
@miralshah365 miralshah365 force-pushed the 2d_convolution branch 2 times, most recently from 46787c9 to 9a008e0 Compare Aug 5, 2019
@mloskot mloskot added this to In progress in [gsoc2019] Image Processing Algorithms via automation Aug 5, 2019
@mloskot mloskot marked this pull request as ready for review Aug 5, 2019
@mloskot
Copy link
Member

@mloskot mloskot commented Aug 6, 2019

@miralshah365 Do you plan to work on more test cases?

@miralshah365 miralshah365 force-pushed the 2d_convolution branch 2 times, most recently from fe65306 to 7eff290 Compare Aug 6, 2019
@miralshah365 miralshah365 requested a review from mloskot Aug 6, 2019
test/extension/numeric/Jamfile Outdated Show resolved Hide resolved
Copy link
Member

@mloskot mloskot left a comment

Generally it looks good. I just have several minor requests.

Could you add comment documenting any constraints and limitations of the current convolve_2d implementation. That is:

image
(Source http://www.songho.ca/dsp/convolution/convolution2d_example.html)

In particular, how it handles the boundary pixels. Ideally, if referenced to/contrasted with/named after the convolve_1d modes:

enum /*class*/ convolve_boundary_option
{
convolve_option_output_ignore, /// do nothing to the output
convolve_option_output_zero, /// set the output to zero
convolve_option_extend_padded, /// assume the source boundaries to be padded already
convolve_option_extend_zero, /// assume the source boundaries to be zero
convolve_option_extend_constant /// assume the source boundaries to be the boundary value
};

I hope it makes sense.

include/boost/gil/extension/numeric/convolve.hpp Outdated Show resolved Hide resolved
include/boost/gil/extension/numeric/convolve.hpp Outdated Show resolved Hide resolved
include/boost/gil/extension/numeric/convolve.hpp Outdated Show resolved Hide resolved
test/extension/numeric/convolve_2d.cpp Outdated Show resolved Hide resolved
test/extension/numeric/convolve_2d.cpp Outdated Show resolved Hide resolved
test/extension/numeric/convolve_2d.cpp Outdated Show resolved Hide resolved
test/extension/numeric/convolve_2d.cpp Outdated Show resolved Hide resolved
test/extension/numeric/convolve_2d.cpp Outdated Show resolved Hide resolved
@miralshah365
Copy link
Contributor Author

@miralshah365 miralshah365 commented Aug 7, 2019

Generally it looks good. I just have several minor requests.

Could you add comment documenting any constraints and limitations of the current convolve_2d implementation. That is:

image
(Source http://www.songho.ca/dsp/convolution/convolution2d_example.html)

In particular, how it handles the boundary pixels. Ideally, if referenced to/contrasted with/named after the convolve_1d modes:

enum /*class*/ convolve_boundary_option
{
convolve_option_output_ignore, /// do nothing to the output
convolve_option_output_zero, /// set the output to zero
convolve_option_extend_padded, /// assume the source boundaries to be padded already
convolve_option_extend_zero, /// assume the source boundaries to be zero
convolve_option_extend_constant /// assume the source boundaries to be the boundary value
};

I hope it makes sense.

The current implementation of convolve_2d is limited to use convolve_option_extend_zero to handle the boundary pixels. This can be considered as one of the limitations of the function unlike convolve_row/col/1d it does not allow various boundary options.

@mloskot mloskot added this to the Boost 1.72+ milestone Aug 7, 2019
@mloskot mloskot removed the status/work-in-progress label Aug 7, 2019
mloskot
mloskot approved these changes Aug 7, 2019
Copy link
Member

@mloskot mloskot left a comment

@miralshah365 LGTM! Feel free to merge as soon as all CI-s are green.

} //namespace detail

/*!
convolve_2d can only use convolve_option_extend_zero as convolve_boundary_option
Copy link
Member

@mloskot mloskot Aug 8, 2019

Choose a reason for hiding this comment

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

@miralshah365 Please, don't forget to open an issue with list your ideas and wishes about what can be improved in the convolve_2d. It will be useful record.

Copy link
Member

@mloskot mloskot left a comment

@miralshah365 Let's resolve this new discussion with @lpranam before merging #367 (review)

Also, you need to update your branch and resolve the little conflict in the build scripts.

2D convolution tests added

`convolve` function renamed to `convolve_1d`

closes boostorg#356
mloskot
mloskot approved these changes Aug 8, 2019
Copy link
Member

@mloskot mloskot left a comment

@miralshah365 LGTM. Feel free to merge.

@lpranam Thank you for offering valuable feedback!

@mloskot
Copy link
Member

@mloskot mloskot commented Aug 8, 2019

@miralshah365 also please don't forget about #367 (comment)

@miralshah365 miralshah365 merged commit ca696ce into boostorg:develop Aug 8, 2019
9 checks passed
[gsoc2019] Image Processing Algorithms automation moved this from In progress to Done Aug 8, 2019
@mloskot mloskot changed the title Added 2D convolution definitions to numeric extension Add convolve_2d 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/feature
Development

Successfully merging this pull request may close these issues.

3 participants