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

[Merged by Bors] - refactor(LinearAlgebra) : Remove unused commutativity hypothesis #9475

Closed
wants to merge 4 commits into from

Conversation

mans0954
Copy link
Collaborator

@mans0954 mans0954 commented Jan 5, 2024

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


Used in #9334

Open in Gitpod

@mans0954 mans0954 marked this pull request as ready for review January 5, 2024 23:14
@mans0954 mans0954 added the awaiting-review The author would like community review of the PR label Jan 5, 2024
@ericrbg
Copy link
Collaborator

ericrbg commented Jan 5, 2024

It would also be possible to weaken commutativity from Rₗ in LinearMap.sum_repr_mul_repr_mul to [SMulCommClass Rₗ Rₗ Pₗ] but I have not done that in this PR.

If you don't want to do this, can you add a comment to this effect?

maintainer merge

Copy link

github-actions bot commented Jan 5, 2024

🚀 Pull request has been placed on the maintainer queue by ericrbg.

mans0954 and others added 2 commits January 6, 2024 04:42
Co-authored-by: Eric Wieser <wieser.eric@gmail.com>
@mans0954
Copy link
Collaborator Author

mans0954 commented Jan 6, 2024

If you don't want to do this, can you add a comment to this effect?

Done.

@jcommelin
Copy link
Member

!bench

@leanprover-bot
Copy link
Collaborator

Here are the benchmark results for commit ab9623c.
There were no significant changes against commit 26ccd1e.

@jcommelin
Copy link
Member

bors merge

@github-actions github-actions bot added ready-to-merge This PR has been sent to bors. and removed awaiting-review The author would like community review of the PR labels Jan 6, 2024
mathlib-bors bot pushed a commit that referenced this pull request Jan 6, 2024
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 <mans0954@users.noreply.github.com>
Co-authored-by: Christopher Hoskin <christopher.hoskin@overleaf.com>
@mathlib-bors
Copy link

mathlib-bors bot commented Jan 6, 2024

Pull request successfully merged into master.

Build succeeded:

@mathlib-bors mathlib-bors bot changed the title refactor(LinearAlgebra) : Remove unused commutativity hypothesis [Merged by Bors] - refactor(LinearAlgebra) : Remove unused commutativity hypothesis Jan 6, 2024
@mathlib-bors mathlib-bors bot closed this Jan 6, 2024
@mathlib-bors mathlib-bors bot deleted the mans0954/LinearMaps-noncommrings branch January 6, 2024 09:50
mathlib-bors bot pushed a commit that referenced this pull request Jan 8, 2024
…lts from SesquilinearForm (#9485)

Give definitions in `LinearAlgebra/Matrix/BilinearForm` in terms of the equivalent definitions in `LinearAlgebra/Matrix/SesquilinearForm` and derive the `BilinearForm` results as effectively special cases of the equivalent results in `SesquilinearForm`. This reduces the length of  `LinearAlgebra/Matrix/BilinearForm` by over 100 lines.

The aim is to:

* Clarify how results in `BilinearForm` relate to results in `SesquilinearForm`
* Reduce duplication of argument between the two files
* Validate that the results in `SesquilinearForm` are sufficiently general to provide the results in `BilinearForm` in their existing form - in fact, some loosening of the hypothesis in `SesquilinearForm` is required. Further loosening was already applied in #9475



Co-authored-by: Christopher Hoskin <mans0954@users.noreply.github.com>
Co-authored-by: Christopher Hoskin <christopher.hoskin@overleaf.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge This PR has been sent to bors.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants