-
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
[Merged by Bors] - refactor(LinearAlgebra/Matrix/SesquilinearForm): Sesquilinear Maps and Matrices #9334
Conversation
…inearMaps-Matrix1
!bench |
Here are the benchmark results for commit 1ad926c. Benchmark Metric Change
======================================================================
- ~Mathlib.LinearAlgebra.Matrix.SesquilinearForm instructions 6.1% |
!bench |
Here are the benchmark results for commit 6f6a726. Benchmark Metric Change
======================================================================
- ~Mathlib.LinearAlgebra.Matrix.SesquilinearForm instructions 31.4% |
@j-loreaux unfortunately the benchmarks look slightly worse (I'm assuming a larger percentage in red is worse?) |
That's such a small change, and only in a single file, that it's not a big deal. It could even be noise. |
General information:
7 files got slower by at least 10⁹ instructions:
2 files got slower by at least 10%:
No file got faster by at least 10⁹ instructions. No file got faster by at least 10%. |
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 maintainer merge |
🚀 Pull request has been placed on the maintainer queue by j-loreaux. |
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.
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+
✌️ mans0954 can now approve this pull request. To approve and merge a pull request, simply reply with |
Co-authored-by: Anne Baanen <[email protected]>
Co-authored-by: Anne Baanen <[email protected]>
Co-authored-by: Anne Baanen <[email protected]>
Co-authored-by: Anne Baanen <[email protected]>
Co-authored-by: Anne Baanen <[email protected]>
Co-authored-by: Anne Baanen <[email protected]>
bors r+ |
…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 <[email protected]> Co-authored-by: Christopher Hoskin <[email protected]>
Pull request successfully merged into master. Build succeeded: |
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 theDet
section.See also: #9312
Hopefully this will replace #8256, in part at least.