-
Notifications
You must be signed in to change notification settings - Fork 15
Add semiring(A @ B @ C)
that applies semiring to both matmuls
#501
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
Conversation
Codecov Report
@@ Coverage Diff @@
## main #501 +/- ##
==========================================
+ Coverage 99.20% 99.39% +0.19%
==========================================
Files 95 95
Lines 21291 21884 +593
Branches 3999 4138 +139
==========================================
+ Hits 21121 21751 +630
+ Misses 124 75 -49
- Partials 46 58 +12 |
Hey @jim22k @SultanOrazbayev @michelp, this is pretty much ready if you want to take a look--I just need to write the error message when mixing infix notations such as This allows operators for ewise add, ewise mult, and matrix multiplication to apply to all the operands inside the operator when using infix notation such as Mixing ewise infix notation now raises: Applying operators like this also only apply to infix notation. Overall I think this change is for the better. I think the main "gotcha" this introduces is this: >>> C = A @ B
>>> E << min_plus(C @ D) Did they mean
Hence, it's best practice to either be explicit with a semiring, call Unless we want to detect whether an expression is assigned or inlined (which is possible, but let's not introduce that voodoo magic if we don't absolutely need to), this is the tradeoff we are stuck with. I think the multi-infix notation can be used well, as @alugowski did in #498. Edge cases exist here no matter what we do (I think), and I'm inclined to support what multiple users expected to work, which is what this PR does. This PR goes further by detecting and disallowing other syntax that is likely to be incorrect. When we discussed this possibility before in #132, @ParticularMiner liked applying the operator to all infix operands as done in this PR (see: #132 (comment)) |
This PR should now be easier to review b/c I added unrelated changes into main via #517. |
I'm okay with this approach. I agree that |
This is in--thanks all! I found it nice to use this syntax when creating #518 (I wrote |
This is an experiment and possible beginning of implementation to address #132 and #498. This allows a semiring to be applied to many matrix multiplies, such as
gb.op.plus_plus(A @ B @ C @ D)
.I believe this pattern does arise in practice, and I expect this to be used in a way that is readable and intuitive. Currently, this:
is equivalent to:
which may be surprising and counterintuitive.
TODO:
binaryop(A | B & C)
(do not support for now)