-
Notifications
You must be signed in to change notification settings - Fork 34
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
Use tuples in KernelSum and KernelProduct; Make Zygote tests pass for KernelSum and KernelProduct; Improve doctring. #146
Conversation
This is an excellent opportunity to provide an docstring demonstrating the various ways to construct these kernels / providing advice regarding when to use which options. In particular
Preferably the examples would be included as doctests to prevent documentation-regression. |
I'm wondering if we should remove the weights in |
What about what @devmotion was saying about for large collections of kernels, tuples would be inefficient? |
I agree for the weights problem we should remove them |
Maybe it's also the good place to check that Zygote tests are passing! |
There seems to be a dependency error with Julia 1.3 build. Any idea what this is about? |
Maybe it was due to https://discourse.julialang.org/t/pkg-downtime-incident/44288 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lightly reviewed for style / docstrings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just a final docstring suggestion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is excellent. Really glad to have it in!
Removing the weight fields is breaking, so could you please bump the minor-version and release :)
#116 (comment)
@devmotion @theogf