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] - chore(RingTheory/TensorProduct): golf #7539

Closed
wants to merge 7 commits into from

Conversation

eric-wieser
Copy link
Member

@eric-wieser eric-wieser commented Oct 6, 2023

This adds LinearMap.map_mul_iff to match AddMonoidHom.map_mul_iff and uses it (along with AlgHom.ofLinearMap) to golf away some induction proof in favor of ext.
The main motivation is the map_mul_iff lemma itself, not the golfing.

Also fixes some incorrect docstrings that were mangled during porting, and adds an explicit name to an instance.


Open in Gitpod

…`AlgHom.ofLinearMap`

The former previously took a hypothesis about `f (algebraMap R A r) = algebraMap R B r`, but now needs only `f 1 = 1`, matching the latter.
This doesn't make much difference at the two callers.
This adds `LinearMap.map_mul_iff` to match the existing `AddMonoidHom.map_mul_iff`, and uses it to golf some results.
@eric-wieser eric-wieser added awaiting-review The author would like community review of the PR awaiting-CI t-algebra Algebra (groups, rings, fields etc) labels Oct 6, 2023
@leanprover-community-mathlib4-bot leanprover-community-mathlib4-bot added the blocked-by-other-PR This PR depends on another PR which is still in the queue. label Oct 6, 2023
@leanprover-community-mathlib4-bot leanprover-community-mathlib4-bot removed the blocked-by-other-PR This PR depends on another PR which is still in the queue. label Oct 9, 2023
@leanprover-community-mathlib4-bot
Copy link
Collaborator

Copy link
Contributor

@ocfnash ocfnash left a comment

Choose a reason for hiding this comment

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

Thanks, even if they weren't your motivation, the golfs are great!

bors d+

Comment on lines +599 to +600
letI : Algebra R C := RestrictScalars.algebra R S C
letI : IsScalarTower R S C := RestrictScalars.isScalarTower R S C
Copy link
Contributor

Choose a reason for hiding this comment

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

Should these be just plain lets?

Suggested change
letI : Algebra R C := RestrictScalars.algebra R S C
letI : IsScalarTower R S C := RestrictScalars.isScalarTower R S C
let _i1 : Algebra R C := RestrictScalars.algebra R S C
let _i2 : IsScalarTower R S C := RestrictScalars.isScalarTower R S C

(Even after reading the tactic documentation I don't fully understand when I'll need to the inlining that letI provides.)

Copy link
Member Author

Choose a reason for hiding this comment

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

This was a combination of lean 3 muscle memory, and an unwillingness to have to name these.

I suspect that there are still good reasons to use letI for instances as a general rule, but indeed it makes no difference here

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm going to leave these as letIs because it matches the mathportisms that are already endemic to mathlib4; but as you saw I created https://leanprover.zulipchat.com/#narrow/stream/287929-mathlib4/topic/let.20vs.20letI.20for.20local.20instances/near/396246664 to revisit this.

@bors
Copy link

bors bot commented Oct 12, 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.

@leanprover-community-mathlib4-bot leanprover-community-mathlib4-bot added delegated and removed awaiting-review The author would like community review of the PR labels Oct 12, 2023
@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 Oct 12, 2023
bors bot pushed a commit that referenced this pull request Oct 12, 2023
This adds `LinearMap.map_mul_iff` to match `AddMonoidHom.map_mul_iff` and uses it (along with `AlgHom.ofLinearMap`) to golf away some induction proof in favor of `ext`.
The main motivation is the `map_mul_iff` lemma itself, not the golfing.

Also fixes some incorrect docstrings that were mangled during porting, and adds an explicit name to an instance.
@bors
Copy link

bors bot commented Oct 12, 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 chore(RingTheory/TensorProduct): golf [Merged by Bors] - chore(RingTheory/TensorProduct): golf Oct 12, 2023
@bors bors bot closed this Oct 12, 2023
@bors bors bot deleted the eric-wieser/LinearMap.map_mul_iff branch October 12, 2023 10:52
grunweg pushed a commit that referenced this pull request Oct 13, 2023
This adds `LinearMap.map_mul_iff` to match `AddMonoidHom.map_mul_iff` and uses it (along with `AlgHom.ofLinearMap`) to golf away some induction proof in favor of `ext`.
The main motivation is the `map_mul_iff` lemma itself, not the golfing.

Also fixes some incorrect docstrings that were mangled during porting, and adds an explicit name to an instance.
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

3 participants