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

apply_kernel: clarify documentation #69

Closed
jdries opened this issue Aug 20, 2019 · 1 comment · Fixed by #118
Closed

apply_kernel: clarify documentation #69

jdries opened this issue Aug 20, 2019 · 1 comment · Fixed by #118

Comments

@jdries
Copy link
Contributor

jdries commented Aug 20, 2019

Some feedback after implementing apply_kernel:

  • There actually seem to be multiple ways in which a kernel can be applied, but I believe we are referring to a 2D convolution. This can be mentioned more explicitly. Probably may make sense to reference more elaborate explanations like: http://www.songho.ca/dsp/convolution/convolution2d_example.html
  • The spec says that the kernel should have the same dimensions as the datacube. The most logical case however is to have a spatial 2D kernel, and to apply that kernel on the 2 spatial dimensions. We can then also allow backends to support xD kernels, if they want to do that. This reduces complexity for the user and backend implementors, as they don't have to worry about higher dimensional kernels.
  • Applying a kernel is only properly defined if the kernel has uneven dimension. Currently my implementation throws an exception when trying for instance to use a 2x2 kernel. The docs can mention this, or we should come up with a convention on how to handle kernels with even dimensions.
@m-mohr m-mohr added this to the v1.0 milestone Dec 13, 2019
@m-mohr m-mohr added documentation help wanted Extra attention is needed labels Dec 13, 2019
@m-mohr m-mohr added accepted and removed help wanted Extra attention is needed labels Jan 13, 2020
@m-mohr
Copy link
Member

m-mohr commented Jan 20, 2020

Solved in PR #118

@m-mohr m-mohr closed this as completed Jan 20, 2020
@m-mohr m-mohr linked a pull request Jul 1, 2020 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants