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

feat(linear_algebra/matrix): define the trace of a square matrix #1883

Merged
merged 11 commits into from
Jan 26, 2020
Merged

feat(linear_algebra/matrix): define the trace of a square matrix #1883

merged 11 commits into from
Jan 26, 2020

Conversation

ocfnash
Copy link
Collaborator

@ocfnash ocfnash commented Jan 15, 2020

TO CONTRIBUTORS:

Make sure you have:

  • reviewed and applied the coding style: coding, naming
  • reviewed and applied the documentation requirements
  • for tactics:
  • make sure definitions and lemmas are put in the right files
  • make sure definitions and lemmas are not redundant

If this PR is related to a discussion on Zulip, please include a link in the discussion.

For reviewers: code review check list

Copy link
Member

@jcommelin jcommelin left a comment

Choose a reason for hiding this comment

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

What you've written so far looks great to me. You could however add more simp lemmas for special cases: trace_zero, trace_one, trace_scalar etc... (edit: trace_neg)

@sgouezel
Copy link
Collaborator

sgouezel commented Jan 16, 2020

I am not sure we need to have both trace and trace_hom: I would just go for the linear map version, and call it trace. Then all the simp lemmas trace_zero, trace_neg and so on become unnecessary, as they are already there, available for all linear functions.

diag should probably also be registered only as a linear map, by the way.

@sgouezel sgouezel added the awaiting-author A reviewer has asked the author a question or requested changes label Jan 16, 2020
@jcommelin
Copy link
Member

Sure, but trace_one and trace_scalar won't be automatic.

@ocfnash
Copy link
Collaborator Author

ocfnash commented Jan 16, 2020

I am not sure we need to have both trace and trace_hom: I would just go for the linear map version, and call it trace. Then all the simp lemmas trace_zero, trace_neg and so on become unnecessary, as they are already there, available for all linear functions.

diag should probably also be registered only as a linear map, by the way.

I will happily reorganise you see fit but I was attempting to cater for matrices whose entries do not necessarily belong to a ring, (e.g., I would like this to work for a matrix of natural numbers).

@kbuzzard
Copy link
Member

@ocfnash
Copy link
Collaborator Author

ocfnash commented Jan 19, 2020

Given that it looks like we're not going to have linearity over semirings all that soon, I guess we have to choose between either:

  • Having general trace, trace_hom as I proposed in my first commit, or
  • Dropping support for matrices with entries of type add_comm_monoid

I have just added this commit which opts for the latter, as requested by @sgouezel

Note that I have thus omitted simp lemmas for trace_zero, trace_scalar, trace_neg since they all follow from the linearity and the included simp lemma trace_one. I.e., the following all compile:

lemma trace_zero [decidable_eq n] [ring R] :
  (trace : (matrix n n R) →ₗ[R] R) 0 = 0 := by simp

lemma trace_scalar [decidable_eq n] [ring R] (c : R) :
  (trace : (matrix n n R) →ₗ[R] R) (c • 1) = c * (fintype.card n) := by simp

lemma trace_neg [decidable_eq n] [ring R] (M : matrix n n R) :
  (trace : (matrix n n R) →ₗ[R] R) (-M) = -(trace : (matrix n n R) →ₗ[R] R) M := by simp

@ocfnash
Copy link
Collaborator Author

ocfnash commented Jan 19, 2020

Hmm, I guess I've jumped over the half-way house of a matrix of elements of a module but I now note that the following instance does not exist:

instance matrix.module {R : Type v} {M : Type w} [ring R] [add_comm_group M] [module R M] :
  module R (matrix n n M) := infer_instance -- fails

I think I should add this instance (and likewise for semimodules) so this is still WIP I guess.

@ocfnash
Copy link
Collaborator Author

ocfnash commented Jan 20, 2020

I have just pushed a commit to show how things look if we want to have diag and trace defined for module-valued matrices (rather than just ring-valued).

I think this is as far as I can take this without someone making a call on what looks best.

Copy link
Member

@jcommelin jcommelin left a comment

Choose a reason for hiding this comment

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

I think this looks good for now. We can generalise to semirings later.

src/linear_algebra/matrix.lean Outdated Show resolved Hide resolved
Co-Authored-By: Johan Commelin <johan@commelin.net>
Copy link
Member

@jcommelin jcommelin left a comment

Choose a reason for hiding this comment

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

I'm fine with this as it is.

My only remaining question is: do we want to have decidable_eq n or should we go classical?
@rwbarton @ChrisHughes24 @digama0

@ChrisHughes24
Copy link
Member

I would prefer to make matrices classical. They are not a remotely efficient implementation anyway. I think that's a matter for another PR

@ChrisHughes24 ChrisHughes24 added ready-to-merge All that is left is for bors to build and merge this PR. (Remember you need to say `bors r+`.) and removed awaiting-author A reviewer has asked the author a question or requested changes labels Jan 26, 2020
@ChrisHughes24
Copy link
Member

This is ready to merge, but it's not up to date and I can't update the branch because it's protected. @ocfnash could you update the branch or change some protection settings?

@ocfnash
Copy link
Collaborator Author

ocfnash commented Jan 26, 2020

This is ready to merge, but it's not up to date and I can't update the branch because it's protected. @ocfnash could you update the branch or change some protection settings?

Thanks @ChrisHughes24 I have not intentionally enabled any protection settings, and GH claims to me that indeed I have none enabled, and yet I saw that GH was refusing to allow the merge. 🤷‍♂ In any case, I have merged latest master into this branch manually so hopefully that will unblock things.

@ocfnash
Copy link
Collaborator Author

ocfnash commented Jan 26, 2020

Argh, still no success merging. On further inspection I have discovered (and ticked) the "Allow edits from maintainers" box:
image
So hopefully things should work now.

@mergify mergify bot merged commit 497e692 into leanprover-community:master Jan 26, 2020
@ocfnash ocfnash deleted the matrix_trace branch January 26, 2020 22:04
ocfnash pushed a commit that referenced this pull request Jan 27, 2020
In a striking coincidence,
  #1910
was merged almost immediately before
  #1883
thus rendering matrix.smul_sum redundant.
mergify bot added a commit that referenced this pull request Jan 31, 2020
…matrices. (#1913)

* feat(linear_algebra/matrix): trace AB = trace BA

* Remove now-redundant matrix.smul_sum

In a striking coincidence,
  #1910
was merged almost immediately before
  #1883
thus rendering matrix.smul_sum redundant.

* Make arguments explicit for matrix.trace, matrix.diag

* Tidy up whitespace

* Remove now-redundant type ascriptions

* Update src/linear_algebra/matrix.lean

Co-Authored-By: Johan Commelin <johan@commelin.net>

* Feedback from code review

* Generalize diag_transpose, trace_transpose.

With apologies to the CI for triggering another build :-/

* Explicit arguments trace, diag defs but not lemmas

Co-authored-by: Johan Commelin <johan@commelin.net>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
anrddh pushed a commit to anrddh/mathlib that referenced this pull request May 15, 2020
…nprover-community#1883)

* feat(linear_algebra/matrix): define the trace of a square matrix

* Move ring carrier to correct universe

* Add lemma trace_one, and define diag as linear map

* Define diag and trace solely as linear functions

* Diag and trace for module-valued matrices

* Fix cyclic import

* Rename matrix.mul_sum' --> matrix.smul_sum

Co-Authored-By: Johan Commelin <johan@commelin.net>

* Trigger CI

Co-authored-by: Johan Commelin <johan@commelin.net>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
anrddh pushed a commit to anrddh/mathlib that referenced this pull request May 15, 2020
…matrices. (leanprover-community#1913)

* feat(linear_algebra/matrix): trace AB = trace BA

* Remove now-redundant matrix.smul_sum

In a striking coincidence,
  leanprover-community#1910
was merged almost immediately before
  leanprover-community#1883
thus rendering matrix.smul_sum redundant.

* Make arguments explicit for matrix.trace, matrix.diag

* Tidy up whitespace

* Remove now-redundant type ascriptions

* Update src/linear_algebra/matrix.lean

Co-Authored-By: Johan Commelin <johan@commelin.net>

* Feedback from code review

* Generalize diag_transpose, trace_transpose.

With apologies to the CI for triggering another build :-/

* Explicit arguments trace, diag defs but not lemmas

Co-authored-by: Johan Commelin <johan@commelin.net>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
anrddh pushed a commit to anrddh/mathlib that referenced this pull request May 16, 2020
…nprover-community#1883)

* feat(linear_algebra/matrix): define the trace of a square matrix

* Move ring carrier to correct universe

* Add lemma trace_one, and define diag as linear map

* Define diag and trace solely as linear functions

* Diag and trace for module-valued matrices

* Fix cyclic import

* Rename matrix.mul_sum' --> matrix.smul_sum

Co-Authored-By: Johan Commelin <johan@commelin.net>

* Trigger CI

Co-authored-by: Johan Commelin <johan@commelin.net>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
anrddh pushed a commit to anrddh/mathlib that referenced this pull request May 16, 2020
…matrices. (leanprover-community#1913)

* feat(linear_algebra/matrix): trace AB = trace BA

* Remove now-redundant matrix.smul_sum

In a striking coincidence,
  leanprover-community#1910
was merged almost immediately before
  leanprover-community#1883
thus rendering matrix.smul_sum redundant.

* Make arguments explicit for matrix.trace, matrix.diag

* Tidy up whitespace

* Remove now-redundant type ascriptions

* Update src/linear_algebra/matrix.lean

Co-Authored-By: Johan Commelin <johan@commelin.net>

* Feedback from code review

* Generalize diag_transpose, trace_transpose.

With apologies to the CI for triggering another build :-/

* Explicit arguments trace, diag defs but not lemmas

Co-authored-by: Johan Commelin <johan@commelin.net>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
@eric-wieser
Copy link
Member

I am not sure we need to have both trace and trace_hom: I would just go for the linear map version, and call it trace. Then all the simp lemmas trace_zero, trace_neg and so on become unnecessary, as they are already there, available for all linear functions.

diag should probably also be registered only as a linear map, by the way.

Note I ended up reverting this decision (#13687, #13712), it turned out to just be annoying to specify which ring we were talking about trace and diag being linear over, and wasn't consistent with other unbundled linear operations in the rest of the matrix api (diagonal, transpose, etc).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge All that is left is for bors to build and merge this PR. (Remember you need to say `bors r+`.)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants