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(data/equiv/mul_add): use @[simps] #7213

Closed
wants to merge 2 commits into from

Conversation

fpvandoorn
Copy link
Member

Add some @[simps] for some algebra maps. This came up in #6795.


WIP until this builds.

Open in Gitpod

@fpvandoorn fpvandoorn added the WIP Work in progress label Apr 15, 2021
@github-actions github-actions bot added the merge-conflict Please `git merge origin/master` then a bot will remove this label. label Apr 16, 2021
@benjamindavidson
Copy link
Collaborator

Thank you for taking care of this!

@fpvandoorn fpvandoorn added awaiting-review The author would like community review of the PR and removed WIP Work in progress labels Apr 16, 2021
@github-actions github-actions bot removed the merge-conflict Please `git merge origin/master` then a bot will remove this label. label Apr 16, 2021
@@ -389,7 +389,7 @@ def comp_hom : (normed_group_hom V₂ V₃) →+ (normed_group_hom V₁ V₂)
add_monoid_hom.mk' (λ g, add_monoid_hom.mk' (λ f, g.comp f)
(by { intros, ext, exact g.map_add _ _ }))
(by { intros, ext, simp only [comp_apply, pi.add_apply, function.comp_app,
add_monoid_hom.add_apply, add_monoid_hom.coe_mk', coe_add] })
add_monoid_hom.add_apply, add_monoid_hom.mk'_apply, coe_add] })
Copy link
Member

Choose a reason for hiding this comment

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

What is the new statement of mk'_apply?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is the same as the current add_monoid_hom.coe_mk', i.e. an equality between functions. I tried to keep the generated lemmas as close to the existing ones, except for the name.

It is currently not possible in the @[simps] framework to sometimes use a projection with one name _apply and sometimes with a different name _coe (though this would be easy to add).

Copy link
Member

Choose a reason for hiding this comment

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

_apply feels like the wrong name here - I'm not sure we should be using simps to save lines if it comes at the expense of generating an unusual name.

@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 Apr 17, 2021
Comment on lines -491 to -494
@[simp] lemma coe_mul_left' (a : G) (ha : a ≠ 0) : ⇑(equiv.mul_left' a ha) = (*) a := rfl

@[simp] lemma mul_left'_symm_apply (a : G) (ha : a ≠ 0) :
((equiv.mul_left' a ha).symm : G → G) = (*) a⁻¹ := rfl
Copy link
Member

Choose a reason for hiding this comment

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

Do the generated lemmas use λ x, a * x or (*) a? The latter seems more desirable, but I don't know about about simps to know if its what actually happens.

bors bot pushed a commit that referenced this pull request Apr 17, 2021
Add some `@[simps]` for some algebra maps. This came up in #6795.
@bors
Copy link

bors bot commented Apr 17, 2021

Build failed (retrying...):

bors bot pushed a commit that referenced this pull request Apr 17, 2021
Add some `@[simps]` for some algebra maps. This came up in #6795.
@bors
Copy link

bors bot commented Apr 17, 2021

Build failed (retrying...):

bors bot pushed a commit that referenced this pull request Apr 17, 2021
Add some `@[simps]` for some algebra maps. This came up in #6795.
@bryangingechen
Copy link
Collaborator

Failure was due to elan, should hopefully be fixed now.
bors r+

@bors
Copy link

bors bot commented Apr 17, 2021

Already running a review

@bors
Copy link

bors bot commented Apr 17, 2021

Pull request successfully merged into master.

Build succeeded:

@bors bors bot changed the title feat(data/equiv/mul_add): use @[simps] [Merged by Bors] - feat(data/equiv/mul_add): use @[simps] Apr 17, 2021
@bors bors bot closed this Apr 17, 2021
@bors bors bot deleted the mul_add_simps branch April 17, 2021 19:14
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

5 participants