-
Notifications
You must be signed in to change notification settings - Fork 74
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
Non-commutative inner product support #437
Conversation
Add separate `MultiVector.__ror__` (based on `__or__`), to calculate inner product from swapped operands. This is NOT commutative operation.
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 looks good to me; can you add a test?
Add test to cover swapped operand operations on MultiVector-s. Esp. the fix for pygae#426.
I added a Possibly, this is a little bit of overshoot, please comment. |
Hi @arsenovic, |
Add "noqa: F811" comment similar to other tests with `rng`.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #437 +/- ##
==========================================
- Coverage 71.35% 71.13% -0.23%
==========================================
Files 73 74 +1
Lines 9461 9437 -24
Branches 1313 1211 -102
==========================================
- Hits 6751 6713 -38
- Misses 2539 2551 +12
- Partials 171 173 +2 ☔ View full report in Codecov by Sentry. |
operator.or_, # inner product | ||
]) | ||
def test_swapped_operands(self, algebra, rng, func): # noqa: F811 | ||
layout = algebra |
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.
Probably it's sufficient to just test g3
here, assuming that elements with non-commutative inner products exist there.
mv = layout.randomMV(rng=rng) | ||
mv2 = layout.randomMV(rng=rng) |
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.
Similarly, I'd be tempted to just pick two elements here on which all the products are non-commutative.
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.
Is that ok: trundev@b2418f0
I did not push this in
fix-pygae-426
branch to avoid history pollution (if something is not ok, it can be simply amended).
BTW, there is still no test coverage for the second return
inside __ror__
(similar in __or__
): https://github.com/trundev/clifford/blob/dcaa727/clifford/_multivector.py#L233
I'm not sure what cases it is intended to handle. Scalar multiplication is handled as if it is a multi-vector (converted by _checkOther
).
@eric-wieser, please note that the "Simplify swapped operand operations pytest" commit was NOT included here. |
If you've already written the code, please do! |
Add separate
MultiVector.__ror__
(based on__or__
), to calculate inner product from swapped operands.The inner product is NOT a commutative operation.\
Fixes #426