Add KernelTensorSum#507
Add KernelTensorSum#507martincornejo wants to merge 11 commits intoJuliaGaussianProcesses:masterfrom
KernelTensorSum#507Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #507 +/- ##
===========================================
- Coverage 94.16% 64.78% -29.38%
===========================================
Files 52 53 +1
Lines 1387 1414 +27
===========================================
- Hits 1306 916 -390
- Misses 81 498 +417 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
willtebbutt
left a comment
willtebbutt
left a comment
There was a problem hiding this comment.
Thanks for opening this PR. It's looking pretty good -- there are a few details I would like addressed before merging though
|
The failing tests introduced with Julia 1.9 are not related to the new Kernel. How should I proceed? |
|
I've renamed |
|
Hi, is there anything holding this PR? Let me know if I should do anything more from my side. Thanks! |
src/kernels/overloads.jl
Outdated
| @@ -1,7 +1,10 @@ | |||
| function ⊕ end | |||
There was a problem hiding this comment.
This seems too generic to be defined and exported from KernelFunctions. Is it not part of TensorCore or some other lightweight interface package? We would also a non-Unicode alias, as for other keyword arguments and functions.
There was a problem hiding this comment.
I thought about that, but ⊕ is not part of TensorCore or as far as I know any other lightweight package (https://juliahub.com/ui/Search?q=%E2%8A%95&type=symbols). It is a help constructor for the new KernelTensorSum/KernelIndependentSum, so the non-Unicode function is already available.
There was a problem hiding this comment.
Suggestions wellcome on how to improve this.
There was a problem hiding this comment.
There's no non-Unicode alternative similar to +, *, or tensor yet as far as I can tell?
There was a problem hiding this comment.
This seems too generic to be defined and exported from KernelFunctions.
This problem is not fixed yet, is it?
There was a problem hiding this comment.
But since TensorCore.jl does not define ⊕, what should we do? Here are the packages that use ⊕. Kronecker.jl is one, but I guess we do not want to add this as a dependency, only to re-use the symbol.
There was a problem hiding this comment.
We should make a PR to TensorCore. I think the operator should not be owned by KernelFunctions.
There was a problem hiding this comment.
|
I would have chosen |
|
Sorry for not getting around to reviewing this.
Could you elaborate on why this is a better name? This actually does worry me, because I value consistency with |
That was my initial idea as well, but without the context of But I'm completely open about this, both are valid alternatives. I'll leave the decision up to you. |
|
That's fair. I agree that I'm not entirely sure that the use of the term is correct, but I still think my preference is consistency + an issue suggesting that both be renamed when we next create a breaking change / a proposal to deprecate the tensor names. |
|
I only found one reference to the term "Tensor sum" here: https://www.degruyter.com/document/doi/10.1515/spma-2019-0009/html?lang=en |
|
The term "tensor sum" is also used in this proposal: JuliaLang/julia#13333 (even though arguably "direct sum" is more common - but not always synonymous: JuliaLang/julia#13333 (comment)) |
4 participants
Adressess #506. Implements a new kernel composition
KernelTensorSumand related⊕operator.The naming should be discussed since the meaning of
KernelTensorSummight not be appropriate. Suggestions are welcome.