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] - feat(LinearAlgebra/Matrix): Woodbury Identity #16325

Closed
wants to merge 21 commits into from

Conversation

4hma4d
Copy link
Collaborator

@4hma4d 4hma4d commented Aug 30, 2024

This adds the Woodbury Identity.

Zulip discussion

Also corrects some bad deprecations introduced in #16590, which affected development of this PR.

Co-authored-by: Mohanad Ahmad


Open in Gitpod

@4hma4d 4hma4d added easy < 20s of review time. See the lifecycle page for guidelines. t-algebra Algebra (groups, rings, fields, etc) labels Aug 30, 2024
@github-actions github-actions bot added the new-contributor This PR was made by a contributor with fewer than 5 merged PRs. Welcome to the community! label Aug 30, 2024
Copy link

github-actions bot commented Aug 30, 2024

PR summary d51e3a90ee

Import changes for modified files

Dependency changes

File Base Count Head Count Change
Mathlib.Data.Matrix.Invertible 803 805 +2 (+0.25%)
Import changes for all files
Files Import difference
Mathlib.Data.Matrix.Invertible 2

Declarations diff

+ add_mul_mul_invOf_mul_eq_one
+ add_mul_mul_invOf_mul_eq_one'
+ add_mul_mul_inv_eq_sub
+ invOf_add_mul_mul
+ invertibleAddMulMul

You can run this locally as follows
## summary with just the declaration names:
./scripts/declarations_diff.sh <optional_commit>

## more verbose report:
./scripts/declarations_diff.sh long <optional_commit>

The doc-module for script/declarations_diff.sh contains some details about this script.

@vihdzp vihdzp removed the easy < 20s of review time. See the lifecycle page for guidelines. label Sep 1, 2024
Mathlib/LinearAlgebra/Matrix/NonsingularInverse.lean Outdated Show resolved Hide resolved
variable (A: Matrix n n α) (U : Matrix n m α) (C : Matrix m m α) (V : Matrix m n α)
variable [Invertible A] [Invertible C][Invertible (C⁻¹ + V * A⁻¹* U)]

lemma add_mul_mul_inv_mul_eq_one: (A+U*C*V)*(A⁻¹-A⁻¹*U*(C⁻¹+V*A⁻¹*U)⁻¹*V*A⁻¹) = 1 := by
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
lemma add_mul_mul_inv_mul_eq_one: (A+U*C*V)*(A⁻¹-A⁻¹*U*(C⁻¹+V*A⁻¹*U)⁻¹*V*A⁻¹) = 1 := by
lemma add_mul_mul_inv_mul_eq_one: (A + U * C * V) * (A⁻¹ - A⁻¹ * U * (C⁻¹ + V * A⁻¹ * U)⁻¹ * V * A⁻¹) = 1 := by

Can you also properly space out the rest of the proof?

Copy link
Member

Choose a reason for hiding this comment

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

I think it might be worth deviating from the "spaces aroung *" style here, and only adding them around the additive operators.

Mathlib/LinearAlgebra/Matrix/NonsingularInverse.lean Outdated Show resolved Hide resolved
Mathlib/LinearAlgebra/Matrix/NonsingularInverse.lean Outdated Show resolved Hide resolved
4hma4d and others added 2 commits September 4, 2024 23:27
Co-authored-by: Violeta Hernández <vi.hdz.p@gmail.com>
@eric-wieser eric-wieser added the awaiting-author A reviewer has asked the author a question or requested changes label Sep 7, 2024
variable (A: Matrix n n α) (U : Matrix n m α) (C : Matrix m m α) (V : Matrix m n α)
variable [Invertible A] [Invertible C] [Invertible (⅟C + V * ⅟A* U)]

lemma add_mul_mul_invOf_mul_eq_one : (A + U*C*V)*(⅟A - ⅟A*U*⅟(⅟C + V*⅟A*U)*V*⅟A) = 1 := by
Copy link
Member

Choose a reason for hiding this comment

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

Can you move this to Matrix/Invertible.lean, and generalize it from CommRing to Ring?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Should i move all of it or just this lemma?

Copy link
Member

@eric-wieser eric-wieser Sep 10, 2024

Choose a reason for hiding this comment

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

Yes, all the lemmas currently in this PR. The lemma I suggest below about inv would remain in this file.

Copy link
Member

Choose a reason for hiding this comment

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

@4hma4d, do you want me to do the move for you? Happy to do so to get this PR over the line, but don't want to tread on your toes.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sorry for the delay. I didn't move add_mul_mul_inv since it relies on Matrix.invertibleOfRightInverse which is proved in NonsingularInverse

Copy link
Member

Choose a reason for hiding this comment

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

I didn't move add_mul_mul_inv since it relies on Matrix.invertibleOfRightInverse which is proved in NonsingularInverse

Indeed, to prove add_mul_mul_inv for the non-commutative case you have to repeat the whole proof, but with multiplication on the left instead! I've pushed a commit that does that.

@eric-wieser eric-wieser self-assigned this Sep 10, 2024
@MohanadAhmed MohanadAhmed removed the awaiting-author A reviewer has asked the author a question or requested changes label Sep 19, 2024
Comment on lines 131 to 150
-- as above, but with multiplication reversed
lemma add_mul_mul_invOf_mul_eq_one' :
(⅟A - ⅟A*U*⅟(⅟C + V*⅟A*U)*V*⅟A)*(A + U*C*V) = 1 := by
calc
(⅟A - ⅟A*U*⅟(⅟C + V*⅟A*U)*V*⅟A)*(A + U*C*V)
_ = ⅟A*A - ⅟A*U*⅟(⅟C + V*⅟A*U)*V*⅟A*A + ⅟A*U*C*V - ⅟A*U*⅟(⅟C + V*⅟A*U)*V*⅟A*U*C*V := by
simp_rw [add_sub_assoc, _root_.mul_add, _root_.sub_mul, Matrix.mul_assoc]
_ = (1 + ⅟A*U*C*V) - (⅟A*U*⅟(⅟C + V*⅟A*U)*V + ⅟A*U*⅟(⅟C + V*⅟A*U)*V*⅟A*U*C*V) := by
rw [invOf_mul_self, Matrix.mul_invOf_mul_self_cancel]
abel
_ = 1 + ⅟A*U*C*V - ⅟A*U*⅟(⅟C + V*⅟A*U)*(V + V*⅟A*U*C*V) := by
rw [sub_right_inj, Matrix.mul_add]
simp_rw [Matrix.mul_assoc]
_ = 1 + ⅟A*U*C*V - ⅟A*U*⅟(⅟C + V*⅟A*U)*(⅟C + V*⅟A*U)*C*V := by
congr 1
simp only [Matrix.mul_add, Matrix.add_mul, ← Matrix.mul_assoc,
Matrix.mul_invOf_mul_self_cancel]
_ = 1 := by
rw [Matrix.mul_invOf_mul_self_cancel]
abel
Copy link
Member

Choose a reason for hiding this comment

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

I copy-pasted this from your proof above, and tweaked it to reverse all the multiplications. Thanks for writing the original which made this relatively easy!

@4hma4d
Copy link
Collaborator Author

4hma4d commented Sep 24, 2024

Is this fine? I added the spaces in Nonsingularinverse since I don't think it affects clarity there, and I added a comment in Invertible

@eric-wieser
Copy link
Member

maintainer merge

Let's let another maintainer take the final look.
Thanks for your work here @4hma4d!

Copy link

🚀 Pull request has been placed on the maintainer queue by eric-wieser.

@riccardobrasca
Copy link
Member

Thanks!

bors merge

@github-actions github-actions bot added the ready-to-merge This PR has been sent to bors. label Sep 25, 2024
@eric-wieser eric-wieser changed the title feat (LinearAlgebra/Matrix): Woodbury Identity feat(LinearAlgebra/Matrix): Woodbury Identity Sep 25, 2024
mathlib-bors bot pushed a commit that referenced this pull request Sep 25, 2024
@mathlib-bors
Copy link
Contributor

mathlib-bors bot commented Sep 25, 2024

Build failed:

@riccardobrasca
Copy link
Member

Can you please merge master and fix the problem? I think it is just something that is now deprecated.

bors d+

@mathlib-bors
Copy link
Contributor

mathlib-bors bot commented Sep 25, 2024

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

@4hma4d
Copy link
Collaborator Author

4hma4d commented Sep 26, 2024

bors r+

mathlib-bors bot pushed a commit that referenced this pull request Sep 26, 2024
This adds the [Woodbury Identity](https://en.wikipedia.org/wiki/Woodbury_matrix_identity).

[Zulip discussion](https://leanprover.zulipchat.com/#narrow/stream/287929-mathlib4/topic/Woodbury.20identity/near/462245284)

Also corrects some bad deprecations introduced in #16590, which affected development of this PR.

Co-authored-by: Mohanad Ahmad



Co-authored-by: Eric Wieser <wieser.eric@gmail.com>
@mathlib-bors
Copy link
Contributor

mathlib-bors bot commented Sep 26, 2024

Pull request successfully merged into master.

Build succeeded:

@mathlib-bors mathlib-bors bot changed the title feat(LinearAlgebra/Matrix): Woodbury Identity [Merged by Bors] - feat(LinearAlgebra/Matrix): Woodbury Identity Sep 26, 2024
@mathlib-bors mathlib-bors bot closed this Sep 26, 2024
@mathlib-bors mathlib-bors bot deleted the woodbury branch September 26, 2024 03:43
eric-wieser added a commit to eric-wieser/lean-matrix-cookbook that referenced this pull request Sep 28, 2024
eric-wieser added a commit to eric-wieser/lean-matrix-cookbook that referenced this pull request Sep 29, 2024
eric-wieser added a commit to eric-wieser/lean-matrix-cookbook that referenced this pull request Sep 29, 2024
eric-wieser added a commit to eric-wieser/lean-matrix-cookbook that referenced this pull request Sep 29, 2024
eric-wieser added a commit to eric-wieser/lean-matrix-cookbook that referenced this pull request Sep 29, 2024
eric-wieser added a commit to eric-wieser/lean-matrix-cookbook that referenced this pull request Sep 29, 2024
This main result is `Matrix.add_mul_mul_inv_eq_sub` which was added to mathlib in leanprover-community/mathlib4#16325.

Some of these other corollaries could probably be upstreamed.
fbarroero pushed a commit that referenced this pull request Oct 2, 2024
This adds the [Woodbury Identity](https://en.wikipedia.org/wiki/Woodbury_matrix_identity).

[Zulip discussion](https://leanprover.zulipchat.com/#narrow/stream/287929-mathlib4/topic/Woodbury.20identity/near/462245284)

Also corrects some bad deprecations introduced in #16590, which affected development of this PR.

Co-authored-by: Mohanad Ahmad



Co-authored-by: Eric Wieser <wieser.eric@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
delegated maintainer-merge new-contributor This PR was made by a contributor with fewer than 5 merged PRs. Welcome to the community! 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.

6 participants