Skip to content
This repository was archived by the owner on Jul 24, 2024. It is now read-only.

[Merged by Bors] - feat(data/matrix/basic): add missing smul instances, generalize lemmas to work on scalar towers#7544

Closed
eric-wieser wants to merge 6 commits intomasterfrom
eric-wieser/matrix-scalar-tower
Closed

[Merged by Bors] - feat(data/matrix/basic): add missing smul instances, generalize lemmas to work on scalar towers#7544
eric-wieser wants to merge 6 commits intomasterfrom
eric-wieser/matrix-scalar-tower

Conversation

@eric-wieser
Copy link
Copy Markdown
Member

@eric-wieser eric-wieser commented May 7, 2021

This also fixes the add_monoid_hom.map_zero etc lemmas to not require overly strong typeclasses on α

This removes the matrix.smul_apply lemma since pi.smul_apply and smul_eq_mul together replace it.


Open in Gitpod

…s to work on scalar towers

This also fixes the `add_monoid_hom.map_zero` etc lemmas to not require overly strong typeclasses on `α`
@eric-wieser eric-wieser requested a review from Vierkantor May 7, 2021 09:49
Comment on lines +406 to 412
section homs

-- TODO: there should be a way to avoid restating these for each `foo_hom`.
/-- A version of `one_map` where `f` is a ring hom. -/
@[simp] lemma ring_hom_map_one [decidable_eq n]
{β : Type w} [semiring β] (f : α →+* β) :
@[simp] lemma ring_hom_map_one [decidable_eq n] [semiring α] [semiring β] (f : α →+* β) :
(1 : matrix n n α).map f = 1 :=
one_map f.map_zero f.map_one
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

These were previously in a section with [semiring α], which was wrong for most of the lemmas

Comment on lines -438 to -439
/- This line does not type-check without `id` and `: _`. Lean did not recognize that two different
`add_monoid` instances were def-eq -/
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I figured since this showed up in the diff I may as well sort it out - the problem this refers to doesn't seem to happen any more.

@eric-wieser eric-wieser added the awaiting-review The author would like community review of the PR label May 7, 2021
(L ⬝ M).map f = L.map f ⬝ M.map f :=
by { ext, simp [mul_apply, ring_hom.map_sum], }

lemma is_add_monoid_hom_mul_left (M : matrix l m α) :
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do we want bundled versions of these?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Maybe, but I'd argue that's out of scope - these lines are just moved from below

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I can split the PR to make this move clearer if you like?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

No, it's fine.


section star_ring
variables [decidable_eq n] {R : Type*} [semiring R] [star_ring R]
variables [decidable_eq n] [semiring α] [star_ring α]
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hmm, now you are moving from "meaningful variable name" to "generic type variable name". Why?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Because this is the only section of this file to use R for the coefficients of the matrix.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think we should rather change the rest of the file. But in another PR.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I agree, although I'm not sure what letter I would use for the scalar action type if R were reserved for the coefficients.

Copy link
Copy Markdown
Member

@jcommelin jcommelin May 10, 2021

Choose a reason for hiding this comment

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

In that spot, I would use S for the coefficients, or A.

Copy link
Copy Markdown
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.

Thanks 🎉

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 May 10, 2021
bors bot pushed a commit that referenced this pull request May 10, 2021
…s to work on scalar towers (#7544)

This also fixes the `add_monoid_hom.map_zero` etc lemmas to not require overly strong typeclasses on `α`

This removes the `matrix.smul_apply` lemma since `pi.smul_apply` and `smul_eq_mul` together replace it.
@bors
Copy link
Copy Markdown

bors bot commented May 10, 2021

Pull request successfully merged into master.

Build succeeded:

@bors bors bot changed the title feat(data/matrix/basic): add missing smul instances, generalize lemmas to work on scalar towers [Merged by Bors] - feat(data/matrix/basic): add missing smul instances, generalize lemmas to work on scalar towers May 10, 2021
@bors bors bot closed this May 10, 2021
@bors bors bot deleted the eric-wieser/matrix-scalar-tower branch May 10, 2021 12:34
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

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.

2 participants