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(Data/Matrix): Eliminate notation in favor of HMul #6487

Closed
wants to merge 35 commits into from

Conversation

eric-wieser
Copy link
Member

@eric-wieser eric-wieser commented Aug 10, 2023

The main difficulty here is that * has a slightly difference precedence to . notably around smul and neg.

The other annoyance is that ↑U ⬝ A ⬝ ↑U⁻¹ : Matrix m m 𝔸 now has to be written U.val * A * (U⁻¹).val in order to typecheck.

A downside of this change to consider: if you have a goal of A * (B * C) = (A * B) * C, mul_assoc now gives the illusion of matching, when in fact Matrix.mul_assoc is needed. Previously the distinct symbol made it easy to avoid this mistake.

On the flipside, there is now no need to rewrite by Matrix.mul_eq_mul all the time (indeed, the lemma is now removed).


Open in Gitpod

@eric-wieser eric-wieser added awaiting-review The author would like community review of the PR awaiting-CI labels Aug 10, 2023
@eric-wieser eric-wieser added the t-algebra Algebra (groups, rings, fields etc) label Aug 10, 2023
@leanprover-community-mathlib4-bot leanprover-community-mathlib4-bot added the merge-conflict The PR has a merge conflict with master, and needs manual merging. label Aug 10, 2023
@eric-wieser eric-wieser removed the merge-conflict The PR has a merge conflict with master, and needs manual merging. label Aug 10, 2023
@leanprover-community-mathlib4-bot leanprover-community-mathlib4-bot added the merge-conflict The PR has a merge conflict with master, and needs manual merging. label Aug 10, 2023
@eric-wieser eric-wieser removed the merge-conflict The PR has a merge conflict with master, and needs manual merging. label Aug 10, 2023
Copy link
Collaborator

@joneugster joneugster left a comment

Choose a reason for hiding this comment

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

To help reviewing, I marked every single line which has a non-trivial change with a comment (marking this). They might not require attention.
"trivial" changes are

  • to * in the context of matrices
  • removing rw [Matrix.mul_eq_mul]
  • prec order -A ⬝ B to -(A * B)

Most other comments are about docstrings that need to be adapted

Comment on lines 141 to 144
-- TODO(mathlib4#6487): fix elaboration so annotation on `A` isn't needed
theorem skewAdjointMatricesLieSubalgebraEquiv_apply (P : Matrix n n R) (h : Invertible P)
(A : skewAdjointMatricesLieSubalgebra J) :
↑(skewAdjointMatricesLieSubalgebraEquiv J P h A) = P⁻¹ ⬝ ↑A ⬝ P := by
↑(skewAdjointMatricesLieSubalgebraEquiv J P h A) = P⁻¹ * (A : Matrix n n R) * P := by
Copy link
Collaborator

@joneugster joneugster Aug 11, 2023

Choose a reason for hiding this comment

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

(marking this)

Comment on lines 201 to 203
-- TODO(mathlib4#6487): fix elaboration so `val` isn't needed
theorem exp_units_conj' (U : (Matrix m m 𝔸)ˣ) (A : Matrix m m 𝔸) :
exp 𝕂 (U⁻¹ A ⬝ U : Matrix m m 𝔸) = U⁻¹ exp 𝕂 A ⬝ U :=
exp 𝕂 ((U⁻¹).val * A * U.val) = (U⁻¹).val * exp 𝕂 A * U.val :=
Copy link
Collaborator

Choose a reason for hiding this comment

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

(marking this)

Comment on lines 192 to 194
-- TODO(mathlib4#6487): fix elaboration so `val` isn't needed
nonrec theorem exp_units_conj (U : (Matrix m m 𝔸)ˣ) (A : Matrix m m 𝔸) :
exp 𝕂 (↑U ⬝ A ⬝ ↑U⁻¹ : Matrix m m 𝔸) = ↑U ⬝ exp 𝕂 A ⬝ ↑U⁻¹ := by
exp 𝕂 (U.val * A * (U⁻¹).val) = U.val * exp 𝕂 A * (U⁻¹).val := by
Copy link
Collaborator

Choose a reason for hiding this comment

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

(marking this)

Mathlib/Analysis/NormedSpace/Spectrum.lean Outdated Show resolved Hide resolved
Mathlib/Analysis/Normed/Group/Basic.lean Outdated Show resolved Hide resolved
Mathlib/LinearAlgebra/Span.lean Outdated Show resolved Hide resolved
Mathlib/Tactic/ScopedNS.lean Outdated Show resolved Hide resolved
Mathlib/Topology/Homotopy/Product.lean Outdated Show resolved Hide resolved
Mathlib/Analysis/Calculus/ContDiff.lean Outdated Show resolved Hide resolved
Mathlib/Analysis/Calculus/FDeriv/Symmetric.lean Outdated Show resolved Hide resolved
eric-wieser and others added 2 commits August 11, 2023 13:41
Co-authored-by: Jon Eugster <eugster.jon@gmail.com>
Co-authored-by: Jon Eugster <eugster.jon@gmail.com>
@eric-wieser
Copy link
Member Author

I've hopefully addressed all the bad docstrings now.

@leanprover-community-mathlib4-bot leanprover-community-mathlib4-bot added the merge-conflict The PR has a merge conflict with master, and needs manual merging. label Aug 12, 2023
@leanprover-community-mathlib4-bot leanprover-community-mathlib4-bot removed the merge-conflict The PR has a merge conflict with master, and needs manual merging. label Aug 14, 2023
@ocfnash
Copy link
Contributor

ocfnash commented Aug 15, 2023

Thank you @eric-wieser for doing all this and @joneugster for the very helpful review.

I note some minor points remain to be fixed after merging master but I've looked over the diffs and I think this leaves us much better off. In particular I'm happy to see no increase in maxHeartbeats (it seems there was previously).

bors d+

@bors
Copy link

bors bot commented Aug 15, 2023

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

@github-actions github-actions bot added delegated and removed awaiting-review The author would like community review of the PR labels Aug 15, 2023
@eric-wieser
Copy link
Member Author

In particular I'm happy to see no increase in maxHeartbeats (it seems there was previously).

I think the increases is just incredibly small; in the previous revision, adding or removing @[simp] was enough to push it over the threshold. In the meantime, someone else bumped the hearbeats by much more!

@eric-wieser
Copy link
Member Author

bors merge

@github-actions github-actions bot added the ready-to-merge This PR has been sent to bors. label Aug 16, 2023
bors bot pushed a commit that referenced this pull request Aug 16, 2023
The main difficulty here is that `*` has a slightly difference precedence to `⬝`. notably around `smul` and `neg`.

The other annoyance is that `↑U ⬝ A ⬝ ↑U⁻¹ : Matrix m m 𝔸` now has to be written `U.val * A * (U⁻¹).val` in order to typecheck.

A downside of this change to consider: if you have a goal of `A * (B * C) = (A * B) * C`, `mul_assoc` now gives the illusion of matching, when in fact `Matrix.mul_assoc` is needed. Previously the distinct symbol made it easy to avoid this mistake.

On the flipside, there is now no need to rewrite by `Matrix.mul_eq_mul` all the time (indeed, the lemma is now removed).
@bors
Copy link

bors bot commented Aug 16, 2023

Pull request successfully merged into master.

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

@bors bors bot changed the title refactor(Data/Matrix): Eliminate notation in favor of HMul [Merged by Bors] - refactor(Data/Matrix): Eliminate notation in favor of HMul Aug 16, 2023
@bors bors bot closed this Aug 16, 2023
@bors bors bot deleted the eric-wieser/down-with-⬝ branch August 16, 2023 10:50
semorrison pushed a commit that referenced this pull request Aug 17, 2023
The main difficulty here is that `*` has a slightly difference precedence to `⬝`. notably around `smul` and `neg`.

The other annoyance is that `↑U ⬝ A ⬝ ↑U⁻¹ : Matrix m m 𝔸` now has to be written `U.val * A * (U⁻¹).val` in order to typecheck.

A downside of this change to consider: if you have a goal of `A * (B * C) = (A * B) * C`, `mul_assoc` now gives the illusion of matching, when in fact `Matrix.mul_assoc` is needed. Previously the distinct symbol made it easy to avoid this mistake.

On the flipside, there is now no need to rewrite by `Matrix.mul_eq_mul` all the time (indeed, the lemma is now removed).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
delegated ready-to-merge This PR has been sent to bors. t-algebra Algebra (groups, rings, fields etc)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants