Skip to content

Comments

Moved kernel creation to name-specific functions.#127

Merged
brendancol merged 2 commits intoxarray-contrib:masterfrom
chase-dwelle:simplify-focal-kernels
Sep 10, 2020
Merged

Moved kernel creation to name-specific functions.#127
brendancol merged 2 commits intoxarray-contrib:masterfrom
chase-dwelle:simplify-focal-kernels

Conversation

@chase-dwelle
Copy link
Contributor

Per @thuydotm , simplified some kernel creation.

  • Removed create_kernel(), now kernels are created by calling their name-specific functions. These are only circle_kernel() and annulus_kernel() at the moment. Custom kernels are any numpy array and therefore don't have a function, e.g., kernel = np.ones((3, 3)).
  • Removed _validate_kernel_shape() and replaced with validate_kernel(), which checks that the kernel is a numpy array and has odd dimensions. This isn't a strict requirement, but I think it makes sense to have the focal point be at the center of a kernel. Perhaps in the future we can accept custom kernels where the focal point is not centered and then pad the kernel in order to center it.
  • Updated tests to reflect these changes.

@brendancol
Copy link
Contributor

Hell yeah! @thuydotm let me know what you think

@thuydotm
Copy link
Contributor

thuydotm commented Sep 4, 2020

  • Removed create_kernel(), now kernels are created by calling their name-specific functions. These are only circle_kernel() and annulus_kernel() at the moment.

circle_kernel() and annulus_kernel() look good to me.

  • Custom kernels are any numpy array and therefore don't have a function, e.g., kernel = np.ones((3, 3)).
  • Removed _validate_kernel_shape() and replaced with validate_kernel(), which checks that the kernel is a numpy array and has odd dimensions.

@chase-dwelle We should have a function name custom_kernel which accepts a numpy array as an argument (we may consider other array-like objects as long as we can validate them). The logic in validate_kernel() should be inside custom_kernel also. For now, the function either raises an error or returns the array itself.

@chase-dwelle
Copy link
Contributor Author

chase-dwelle commented Sep 4, 2020

@chase-dwelle We should have a function name custom_kernel which accepts a numpy array as an argument (we may consider other array-like objects as long as we can validate them). The logic in validate_kernel() should be inside custom_kernel also. For now, the function either raises an error or returns the array itself.

Changed. Agree on accepting any array-like object, but need to figure out if it would break any downstream dependencies, though assume we could just convert it to a numpy array during validation.

@thuydotm
Copy link
Contributor

thuydotm commented Sep 5, 2020

@chase-dwelle the return of custom_kernel needs to be a numpy array for sure.

@brendancol this PR looks good and ready to be merged.

@brendancol brendancol merged commit 53d6a73 into xarray-contrib:master Sep 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants