-
Notifications
You must be signed in to change notification settings - Fork 368
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
refactor(LinearAlgeba/BilinearMap): Weaken CommSemiring
to SMulCommClass
#7538
Conversation
Build failing due to:
Not something I think I can fix? |
That's a thing that happens from time to time, the best you can do is ping maintainers on Zulip so that we can disable and fix the faulty runner. It's the case now so I've restarted the build for you. |
bors d+ |
✌️ mans0954 can now approve this pull request. To approve and merge a pull request, simply reply with |
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.
What's the motivation for this change? Are there any interesting actions of non-commuting elements whose actions commute? I think in various places we avoided this generalization because there was no clear use case, and we expected a performance impact.
!bench
@eric-wieser I can explain what I'm trying to do, and you can tell me if I'm going about it in the right way. When I defined Jordan algebras, I left out quadratic Jordan algebras which are more general as they work without restriction on the characteristic. Now I've got a bit more used to working with Mathlib, I thought I'd see if I could give the quadratic definition, but that requires quadratic maps. Currently, I think Mathlib only has QuadraticForm, but defined over a non-commutative ring. I started working on quadratic forms over commutative rings, but it was quickly apparent that it's almost identical to the quadratic forms theory, and we would end up with two files with almost exactly the same results in them. I started thinking about whether it might be possible to develop the theory of quadratic maps over commutative rings and quadratic forms over non-commutative rings simultaneously, and I came up with:
The idea here being that This seemed to be working, until I got to the SMul section where I needed to define scalar multiplication on You can see my work-in-progress branch here: master...mans0954/quadratic-maps So I guess the options are:
|
I'd envisaged that the correct statement was
which corresponds to |
Note that to replace the existing quadratic forms with your definition, we are first going to need #7152. |
1 similar comment
Note that to replace the existing quadratic forms with your definition, we are first going to need #7152. |
That looks plausible - I think the bilinear map is still |
Yes, I agree with the bilinear map form. However, I don't think |
!bench |
Here are the benchmark results for commit 05a2cf5. Benchmark Metric Change
======================================================================
- ~Mathlib.LinearAlgebra.BilinearMap instructions 12.2%
+ ~Mathlib.LinearAlgebra.DirectSum.TensorProduct instructions -7.8%
- ~Mathlib.LinearAlgebra.TensorPower instructions 6.6% |
Exactly - which brings us back to the purpose of this PR. |
Do you have an example in mind (a choice of |
@eric-wieser Do you also need:
i.e. |
No, that feels like a bad assumption to me as it means Edit: hmm, that seems troublesome anyway... |
@eric-wieser I don't see that |
You're right, and that was what my edit was intended to convey. I think the important question here is my earlier message:
|
Yes, sorry, I got a bit lost in all of this earlier and had my wires crossed. I agree now that I don't need this PR to get a scalar multiplication on However, the next place I get stuck is in composition of a quadratic map with a linear map. Specifically I've opened a draft PR #7569 for the quadratic maps in case it's easier to discuss this there? |
Well worse, doesn't it require the rings to be the same? Note that in the short term you can use I think we should close this PR; I don't think there is anything worth merging here, and the conversation is still useful even if this PR is closed. |
Remove unused commutativity hypothesis: * Removes the requirement for the Semirings to be commutative in `LinearMap.ext_basis` and `LinearMap.sum_repr_mul_repr_mulₛₗ` in `LinearAlgebra/Basis/Bilinear` * Remove the requirement for some Semirings to be commutative in `AuxToLinearMap`, `CommSemiring` and `CommRing` in `LinearAlgebra/Matrix/SesquilinearForm` * In addition, the rings in `CommRing` can just be `Semiring` No changes to the proofs are required. It would also be possible to weaken commutativity from `Rₗ` in `LinearMap.sum_repr_mul_repr_mul` to `[SMulCommClass Rₗ Rₗ Pₗ]` in order to make `sum_repr_mul_repr_mulₛₗ` and `sum_repr_mul_repr_mul` consistent, but I have not done that in this PR because there might be a performance impact (see #7538 (review)). Co-authored-by: Christopher Hoskin <[email protected]> Co-authored-by: Christopher Hoskin <[email protected]>
The results in the
CommSemiring
section ofLinearAlgeba/BilinearMap
don't require the semirings to be commutative. It is sufficient that scalar multiplication in the modules is commutative.