Skip to content

[Merged by Bors] - refactor(LinearAlgebra/Matrix/SesquilinearForm): Sesquilinear Maps and Matrices#9334

Closed
mans0954 wants to merge 108 commits intomasterfrom
mans0954/SesquilinearMaps-Matrix1
Closed

[Merged by Bors] - refactor(LinearAlgebra/Matrix/SesquilinearForm): Sesquilinear Maps and Matrices#9334
mans0954 wants to merge 108 commits intomasterfrom
mans0954/SesquilinearMaps-Matrix1

Conversation

@mans0954
Copy link
Copy Markdown
Collaborator

@mans0954 mans0954 commented Dec 29, 2023

Some of the concepts in LinearAlgebra/Matrix/SesquilinearForm can be generalised from Sesquilinear Forms to Sesquilinear Maps with little or no impact on the rest of Mathlib. This PR makes those generalisations.

A number of results in the ToMatrix section relating composition of maps to matrix multiplication could also be generalised, but they require a more genralised notion of matrix multiplication than we currently have available, so they are not included in the scope of this PR. Similarly for most of the results in the Det section.


See also: #9312

Hopefully this will replace #8256, in part at least.

Open in Gitpod

@mans0954 mans0954 added the WIP Work in progress label Dec 29, 2023
@mans0954 mans0954 marked this pull request as ready for review December 30, 2023 11:20
@mans0954 mans0954 added awaiting-review and removed WIP Work in progress labels Dec 30, 2023
@ghost ghost added the blocked-by-other-PR This PR depends on another PR (this label is automatically managed by a bot) label Jan 5, 2024
@ghost ghost added merge-conflict The PR has a merge conflict with master, and needs manual merging. (this label is managed by a bot) and removed blocked-by-other-PR This PR depends on another PR (this label is automatically managed by a bot) labels Jan 6, 2024
@ghost ghost removed the merge-conflict The PR has a merge conflict with master, and needs manual merging. (this label is managed by a bot) label Jan 6, 2024
@mans0954
Copy link
Copy Markdown
Collaborator Author

mans0954 commented Jan 6, 2024

!bench

@leanprover-bot
Copy link
Copy Markdown
Collaborator

Here are the benchmark results for commit 1ad926c.
There were significant changes against commit 357e37b:

  Benchmark                                        Metric         Change
  ======================================================================
- ~Mathlib.LinearAlgebra.Matrix.SesquilinearForm   instructions     6.1%

@mans0954
Copy link
Copy Markdown
Collaborator Author

!bench

@leanprover-bot
Copy link
Copy Markdown
Collaborator

Here are the benchmark results for commit 6f6a726.
There were significant changes against commit 7052367:

  Benchmark                                        Metric         Change
  ======================================================================
- ~Mathlib.LinearAlgebra.Matrix.SesquilinearForm   instructions    31.4%

@mans0954
Copy link
Copy Markdown
Collaborator Author

@j-loreaux unfortunately the benchmarks look slightly worse (I'm assuming a larger percentage in red is worse?)

@j-loreaux
Copy link
Copy Markdown
Contributor

That's such a small change, and only in a single file, that it's not a big deal. It could even be noise.

@mans0954 mans0954 removed the awaiting-author A reviewer has asked the author a question or requested changes. label Jul 15, 2024
@MichaelStollBayreuth
Copy link
Copy Markdown
Contributor

General information:

lint:                                                  +3.5755 * 10⁹ (+0.0445%)
build:                                                 +49.877 * 10⁹ (+0.0406%)

7 files got slower by at least 10⁹ instructions:

Mathlib.LinearAlgebra.Matrix.SesquilinearForm:         +35.147 * 10⁹ (+31.4%)
Mathlib.LinearAlgebra.Matrix.BilinearForm:             +3.8428 * 10⁹ (+4.73%)
Mathlib.Combinatorics.SimpleGraph.LapMatrix:           +2.9559 * 10⁹ (+12.6%)
Mathlib.LinearAlgebra.Matrix.PosDef:                   +2.1526 * 10⁹ (+4.43%)
Mathlib.LinearAlgebra.QuadraticForm.Basic:             +1.6895 * 10⁹ (+0.926%)
Mathlib.Algebra.Lie.Classical:                         +1.3773 * 10⁹ (+3.76%)
Mathlib.CategoryTheory.Category.Cat.Limit:             +1.3142 * 10⁹ (+1.38%)

2 files got slower by at least 10%:

Mathlib.LinearAlgebra.Matrix.SesquilinearForm:                        +31.4%
Mathlib.Combinatorics.SimpleGraph.LapMatrix:                          +12.6%

No file got faster by at least 10⁹ instructions.

No file got faster by at least 10%.

@j-loreaux
Copy link
Copy Markdown
Contributor

Thanks @MichaelStollBayreuth I'll let someone else decide. But I'm okay with this. The other option would be to have these general versions, and then more specific ones which are abbrevs of these to help Lean with elaboration. I expect that would help, but I'm not positive without trying it.

maintainer merge

@github-actions
Copy link
Copy Markdown

🚀 Pull request has been placed on the maintainer queue by j-loreaux.

@github-actions github-actions bot added the maintainer-merge A reviewer has approved the changed; awaiting maintainer approval. label Jul 15, 2024
Copy link
Copy Markdown
Contributor

@Vierkantor Vierkantor left a comment

Choose a reason for hiding this comment

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

I found a few stylistic issues. Overall I think the slower build time is worth it for the increased generality.

So looks good to me with these changes, thanks!

bors d+

@mathlib-bors
Copy link
Copy Markdown
Contributor

mathlib-bors bot commented Jul 16, 2024

✌️ mans0954 can now approve this pull request. To approve and merge a pull request, simply reply with bors r+. More detailed instructions are available here.

@ghost ghost added the delegated This pull request has been delegated to the PR author (or occasionally another non-maintainer). label Jul 16, 2024
mans0954 and others added 7 commits July 16, 2024 18:27
Co-authored-by: Anne Baanen <Vierkantor@users.noreply.github.com>
Co-authored-by: Anne Baanen <Vierkantor@users.noreply.github.com>
Co-authored-by: Anne Baanen <Vierkantor@users.noreply.github.com>
Co-authored-by: Anne Baanen <Vierkantor@users.noreply.github.com>
Co-authored-by: Anne Baanen <Vierkantor@users.noreply.github.com>
Co-authored-by: Anne Baanen <Vierkantor@users.noreply.github.com>
@mans0954
Copy link
Copy Markdown
Collaborator Author

bors r+

mathlib-bors bot pushed a commit that referenced this pull request Jul 16, 2024
…d Matrices (#9334)

Some of the concepts in `LinearAlgebra/Matrix/SesquilinearForm` can be generalised from Sesquilinear Forms to Sesquilinear Maps with little or no impact on the rest of Mathlib. This PR makes those generalisations.

A number of results in the `ToMatrix` section relating composition of maps to matrix multiplication could also be generalised, but they require a more genralised notion of matrix multiplication than we currently have available, so they are not included in the scope of this PR. Similarly for most of the results in the `Det` section.



Co-authored-by: Christopher Hoskin <mans0954@users.noreply.github.com>
Co-authored-by: Christopher Hoskin <christopher.hoskin@overleaf.com>
@mathlib-bors
Copy link
Copy Markdown
Contributor

mathlib-bors bot commented Jul 16, 2024

Pull request successfully merged into master.

Build succeeded:

@mathlib-bors mathlib-bors bot changed the title refactor(LinearAlgebra/Matrix/SesquilinearForm): Sesquilinear Maps and Matrices [Merged by Bors] - refactor(LinearAlgebra/Matrix/SesquilinearForm): Sesquilinear Maps and Matrices Jul 16, 2024
@mathlib-bors mathlib-bors bot closed this Jul 16, 2024
@mathlib-bors mathlib-bors bot deleted the mans0954/SesquilinearMaps-Matrix1 branch July 16, 2024 19:07
@adomani adomani mentioned this pull request Aug 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

delegated This pull request has been delegated to the PR author (or occasionally another non-maintainer). maintainer-merge A reviewer has approved the changed; awaiting maintainer approval. t-algebra Algebra (groups, rings, fields, etc)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants