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

Add KernelTensorSum #507

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

martincornejo
Copy link

Adressess #506. Implements a new kernel composition KernelTensorSum and related operator.

The naming should be discussed since the meaning of KernelTensorSum might not be appropriate. Suggestions are welcome.

@codecov
Copy link

codecov bot commented May 10, 2023

Codecov Report

Patch coverage has no change and project coverage change: -29.38 ⚠️

Comparison is base (ef6d459) 94.16% compared to head (598fbc7) 64.78%.

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     
Impacted Files Coverage Δ
src/KernelFunctions.jl 100.00% <ø> (ø)
src/kernels/kerneltensorsum.jl 0.00% <0.00%> (ø)
src/kernels/overloads.jl 16.66% <ø> (-83.34%) ⬇️

... and 28 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Member

@willtebbutt willtebbutt left a 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

src/kernels/kerneltensorsum.jl Outdated Show resolved Hide resolved
src/kernels/kerneltensorsum.jl Show resolved Hide resolved
src/kernels/kerneltensorsum.jl Outdated Show resolved Hide resolved
src/kernels/kerneltensorsum.jl Outdated Show resolved Hide resolved
src/kernels/kerneltensorsum.jl Outdated Show resolved Hide resolved
@martincornejo
Copy link
Author

The failing tests introduced with Julia 1.9 are not related to the new Kernel. How should I proceed?

@martincornejo
Copy link
Author

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?

@martincornejo
Copy link
Author

Hi, is there anything holding this PR? Let me know if I should do anything more from my side. Thanks!

@martincornejo martincornejo changed the title Add KernelTensorSum Add KernelIndependentSum May 31, 2023
@@ -1,7 +1,10 @@
function ⊕ end
Copy link
Member

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.

Copy link
Author

@martincornejo martincornejo May 31, 2023

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.

Copy link
Author

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.

Copy link
Member

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?

Copy link
Author

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resolve?

Copy link
Member

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?

Copy link
Author

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.

Copy link
Member

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@devmotion
Copy link
Member

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.

@willtebbutt
Copy link
Member

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.

@martincornejo
Copy link
Author

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.

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.

@willtebbutt
Copy link
Member

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.

@martincornejo martincornejo changed the title Add KernelIndependentSum Add KernelTensorSum May 31, 2023
@theogf
Copy link
Member

theogf commented May 31, 2023

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

@devmotion
Copy link
Member

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))

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.

4 participants