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(algebra/group_action_hom): define equivariant maps #2866
Conversation
In a future PR one might redefine |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
LGTM but it would be nice if someone who understands |
src/algebra/group_action_hom.lean
Outdated
|
||
## Main definitions | ||
|
||
* `mul_action_hom M X Y`, the type of equivariant functions from `X` to `Y`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't know what structures M, X and Y are meant to have here. It makes this whole docstring unusable.
src/algebra/group_action_hom.lean
Outdated
|
||
-/ | ||
|
||
universes u v w u₁ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really need all those universe variables? What happens if you put Type*
everywhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am used to using universe variables in lieu of Type*
because of leanprover-community/lean#214 but it seems like this issue is resolved now.
@[nolint has_inhabited_instance] | ||
structure distrib_mul_action_hom extends A →[M] B, A →+ B. | ||
|
||
/-- Reinterpret an equivariant additive monoid homomorphism as an additive monoid homomorphism. -/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand that docstring and the line below it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the make lint happy by adding a docstring to the automatically generated distrib_mul_action_hom.to_add_monoid_hom
.
cf.
mathlib/src/topology/algebra/open_subgroup.lean
Lines 26 to 27 in c8c1869
/-- Reinterpret an `open_subgroup` as a `subgroup`. -/ | |
add_decl_doc open_subgroup.to_subgroup |
src/algebra/group_action_hom.lean
Outdated
|
||
variables {M A B} | ||
|
||
@[simp] lemma coe_fn_coe (f : A →+[M] B) : ((f : A →+ B) : A → B) = f := rfl |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be norm_cast?
src/algebra/group_action_hom.lean
Outdated
variables {M A B} | ||
|
||
@[simp] lemma coe_fn_coe (f : A →+[M] B) : ((f : A →+ B) : A → B) = f := rfl | ||
@[simp] lemma coe_fn_coe' (f : A →+[M] B) : ((f : A →[M] B) : A → B) = f := rfl |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
norm_cast?
@[simp] lemma map_zero (f : A →+[M] B) : f 0 = 0 := | ||
f.map_zero' | ||
|
||
@[simp] lemma map_add (f : A →+[M] B) (x y : A) : f (x + y) = f x + f y := |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm always uneasy with such simp lemmas. It's not clear to me that the RHS is simpler.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But it's what we do for other "algebraic" maps in the library, and it seems to work quite well. I think that 95% of the time I'm using this direction.
src/algebra/group_action_hom.lean
Outdated
|
||
variables (M) {A} | ||
|
||
/-- The identity map is equivariant. -/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This docstring sounds like it describes a Prop.
src/algebra/group_action_hom.lean
Outdated
|
||
variables {M R S} | ||
|
||
@[simp] lemma coe_fn_coe (f : R →+*[M] S) : ((f : R →+* S) : R → S) = f := rfl |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
norm_cast?
src/algebra/group_action_hom.lean
Outdated
variables {M R S} | ||
|
||
@[simp] lemma coe_fn_coe (f : R →+*[M] S) : ((f : R →+* S) : R → S) = f := rfl | ||
@[simp] lemma coe_fn_coe' (f : R →+*[M] S) : ((f : R →+[M] S) : R → S) = f := rfl |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
norm_cast?
src/algebra/group_action_hom.lean
Outdated
|
||
variables (M) {R} | ||
|
||
/-- The identity map is equivariant. -/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Docstring sounds like a Prop
@PatrickMassot 's |
The use of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kckennylau I think this mostly looks good. If you address the comments, I would say "Let's Get This Merged".
Done @jcommelin |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks 🎉
bors merge
👎 Rejected by label |
bors again |
Pull request successfully merged into master. Build succeeded: |
No description provided.