Add convolve_2d function to numeric extension by miralshah365 · Pull Request #367

Add convolve_2d function to numeric extension by miralshah365 · Pull Request #367 · boostorg/gil

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

mloskot marked this pull request as ready for review

August 5, 2019 21:31

Choose a reason for hiding this comment

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

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:

(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.

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:

(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.

Choose a reason for hiding this comment

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

@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

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.

Choose a reason for hiding this comment

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

@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

Choose a reason for hiding this comment

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

mloskot changed the title ~~Added 2D convolution definitions to numeric extension~~ Add convolve_2d function to numeric extension

Oct 23, 2019