-
Notifications
You must be signed in to change notification settings - Fork 41
Add KernelTensorSum
#507
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
Add KernelTensorSum
#507
Conversation
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:
|
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.
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 KernelTensorSum to KernelIndependentSum since in my opinion it better informs what it is. I could also revert this commit, do you have any opinions on the naming?
Hi, is there anything holding this PR? Let me know if I should do anything more from my side. Thanks!
KernelTensorSum (削除ここまで)KernelIndependentSum (追記ここまで)
src/kernels/overloads.jl
Outdated
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestions wellcome on how to improve this.
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.
There's no non-Unicode alternative similar to +, *, or tensor yet as far as I can tell?
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.
Ah! I see, you're right
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.
Resolve?
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should make a PR to TensorCore. I think the operator should not be owned by KernelFunctions.
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.
I would have chosen KernelTensorSum, for consistency with KernelTensorProduct, similar to how we use the names KernelSum/KernelProduct. But I don't have a strong opinion.
Sorry for not getting around to reviewing this.
I've renamed KernelTensorSum to KernelIndependentSum since in my opinion it better informs what it is. I could also revert this commit, do you have any opinions on the naming?
Could you elaborate on why this is a better name? This actually does worry me, because I value consistency with KernelTensorProduct.
I would have chosen
KernelTensorSum, for consistency withKernelTensorProduct, similar to how we use the namesKernelSum/KernelProduct. But I don't have a strong opinion.
Could you elaborate on why this is a better name? This actually does worry me, because I value consistency with
KernelTensorProduct.
That was my initial idea as well, but without the context of KernelTensorProduct, it is difficult to understand what KernelTensorSum is in itself. Also, is the use of the term "tensor sum" used properly in this context? If not, we would be trading off consistency for "formal correctness".
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.
KernelIndependentSum (削除ここまで)KernelTensorSum (追記ここまで)
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))
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.