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/basic): work around leanprover/lean4#2042 #18696

Closed
wants to merge 13 commits into from

Conversation

eric-wieser
Copy link
Member

@eric-wieser eric-wieser commented Mar 29, 2023

This adjust definitions such that everything is well-behaved in the case that things are unfolded. For each such definition, a lemma is added that replaces the equation lemma. Before this PR, we used

def transpose (M : matrix m n α) : matrix n m α
| x y := M y x

which has the nice behavior (in Lean 3 only) of rw transpose only unfolding the definition when it is of the applied form transpose M i j. If dunfold transpose is used then it becomes the undesirable λ x y, M y x in both Lean versions. After this PR, we use

def transpose (M : matrix m n α) : matrix n m α :=
of $ λ x y, M y x

-- TODO: set as an equation lemma for `transpose`, see mathlib4#3024
@[simp] lemma transpose_apply (M : matrix m n α) (i j) :
  transpose M i j = M j i := rfl

This no longer has the nice rw behavior, but we can't have that in Lean4 anyway (leanprover/lean4#2042). It also makes dunfold insert the of, which is better for type-safety.

This affects

  • matrix.transpose
  • matrix.row
  • matrix.col
  • matrix.diagonal
  • matrix.vec_mul_vec
  • matrix.block_diagonal
  • matrix.block_diagonal'
  • matrix.hadamard
  • matrix.kronecker_map
  • pequiv.to_matrix
  • matrix.circulant
  • matrix.mv_polynomial_X
  • algebra.trace_matrix
  • algebra.embeddings_matrix

While this just adds _apply noise in Lean 3, it is necessary when porting to Lean 4 as there the equation lemma is not generated in the way that we want.

This is hopefully exhaustive; it was found by looking for lines ending in matrix .* followed by a | line


Open in Gitpod

This adjust definitions such that everything is well-behaved in the case that things are unfolded.
For each such definition, a lemma is added that replaces the equation lemma.

While this just adds `_apply` noise in Lean 3, it is necessary when porting to Lean 4 as there the equation lemma is not generated in the way that we want.
@eric-wieser eric-wieser added awaiting-review The author would like community review of the PR awaiting-CI The author would like to see what CI has to say before doing more work. labels Mar 29, 2023
@eric-wieser eric-wieser requested a review from a team as a code owner March 29, 2023 22:16
@eric-wieser eric-wieser added the mathport For compatibility with Lean 4 changes, to simplify porting label Mar 29, 2023
@github-actions github-actions bot removed the awaiting-CI The author would like to see what CI has to say before doing more work. label Mar 30, 2023
@j-loreaux
Copy link
Collaborator

LGTM. I think the procedure for searching for defs involving matrix with pattern matching using regex is the best we can hope for, and it seems relatively reliable to me. In the worst case, if we come across any more while porting we can always change it then, but this should at least allow us to finish data.matrix.basic and move on.

maintainer merge

@github-actions
Copy link

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

@semorrison
Copy link
Collaborator

bors merge

@github-actions github-actions bot 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-review The author would like community review of the PR labels Mar 30, 2023
bors bot pushed a commit that referenced this pull request Mar 30, 2023
This adjust definitions such that everything is well-behaved in the case that things are unfolded. For each such definition, a lemma is added that replaces the equation lemma. Before this PR, we used
```lean
def transpose (M : matrix m n α) : matrix n m α
| x y := M y x
```
which has the nice behavior (in Lean 3 only) of `rw transpose` only unfolding the definition when it is of the applied form `transpose M i j`. If `dunfold transpose` is used then it becomes the undesirable `λ x y, M y x` in both Lean versions. After this PR, we use
```lean
def transpose (M : matrix m n α) : matrix n m α :=
of $ λ x y, M y x

-- TODO: set as an equation lemma for `transpose`, see mathlib4#3024
@[simp] lemma transpose_apply (M : matrix m n α) (i j) :
  transpose M i j = M j i := rfl
```
This no longer has the nice `rw` behavior, but we can't have that in Lean4 anyway (leanprover/lean4#2042). It also makes `dunfold` insert the `of`, which is better for type-safety.

This affects
* `matrix.transpose`
* `matrix.row`
* `matrix.col`
* `matrix.diagonal`
* `matrix.vec_mul_vec`
* `matrix.block_diagonal`
* `matrix.block_diagonal'`
* `matrix.hadamard`
* `matrix.kronecker_map`
* `pequiv.to_matrix`
* `matrix.circulant`
* `matrix.mv_polynomial_X`
* `algebra.trace_matrix`
* `algebra.embeddings_matrix`

While this just adds `_apply` noise in Lean 3, it is necessary when porting to Lean 4 as there the equation lemma is not generated in the way that we want.

This is hopefully exhaustive; it was found by looking for lines ending in `matrix .*` followed by a `|` line
@bors
Copy link

bors bot commented Mar 30, 2023

Pull request successfully merged into master.

Build succeeded:

@bors bors bot changed the title refactor(data/matrix/basic): work around leanprover/lean4#2042 [Merged by Bors] - refactor(data/matrix/basic): work around leanprover/lean4#2042 Mar 30, 2023
@bors bors bot closed this Mar 30, 2023
@bors bors bot deleted the eric-wieser/port-safe-matrix branch March 30, 2023 23:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mathport For compatibility with Lean 4 changes, to simplify porting 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

3 participants