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/monoid_algebra): add missing has_coe_to_fun #4315

Closed
wants to merge 1 commit into from

Conversation

eric-wieser
Copy link
Member

@eric-wieser eric-wieser commented Sep 29, 2020

Also does the same for the additive version semimodule k (add_monoid_algebra k G).


has_coe_to_fun discussed at https://leanprover.zulipchat.com/#narrow/stream/113488-general/topic/Difficultly.20applying.20.60monoid_algebra.60/near/211594391

@eric-wieser eric-wieser added the awaiting-review The author would like community review of the PR label Sep 29, 2020
@eric-wieser eric-wieser changed the title feat(data/monoid_algebra): Allow R ≠ k in semimodule R (monoid_algebra k G) feat(data/monoid_algebra): Allow R ≠ k, add missing has_coe_to_fun Sep 29, 2020
Copy link
Member

@gebner gebner left a comment

Choose a reason for hiding this comment

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

There is still a lot of lemma statements that break when removing the local attribute [reducible] monoid_algebra. Not sure if that should be fixed.

src/data/monoid_algebra.lean Outdated Show resolved Hide resolved
@eric-wieser
Copy link
Member Author

Not sure if that should be fixed.

I'd rather not fix that in this PR, but you're probably right that it likely leads to lots of unhelpful lemmas.

@semorrison
Copy link
Collaborator

I'm not certain that has_coe_to_fun is a good idea. We very specifically do not copy over the has_coe_to_fun instance when we introduce polynomials, and instead introduce the coeff API.

I'm really not sure --- but would it be better to introduce coeff at this level instead?

@eric-wieser
Copy link
Member Author

There are lemmas in this file like https://leanprover-community.github.io/mathlib_docs/find/monoid_algebra.mul_apply which are only useful if a has_coe_to_fun instance exists.

For #4321, I want a has_coe_to_fun so I can write grades x i to get the ith grade of x. I could instead declare a type alias there. Which approach makes more sense to you?

@github-actions github-actions bot added the merge-conflict Please `git merge origin/master` then a bot will remove this label. label Sep 30, 2020
@eric-wieser eric-wieser force-pushed the eric-wieser/monoid_algebra-tweaks branch from 53afbc0 to 9824b22 Compare October 2, 2020 11:58
@github-actions github-actions bot removed the merge-conflict Please `git merge origin/master` then a bot will remove this label. label Oct 2, 2020
@eric-wieser eric-wieser force-pushed the eric-wieser/monoid_algebra-tweaks branch from 9824b22 to c31795f Compare October 2, 2020 15:37
@eric-wieser eric-wieser changed the title feat(data/monoid_algebra): Allow R ≠ k, add missing has_coe_to_fun feat(data/monoid_algebra): add missing has_coe_to_fun Oct 2, 2020
@eric-wieser
Copy link
Member Author

I've moved all the algebra parts of this PR to a new PR, to make this one as simple as possible.

@jcommelin
Copy link
Member

For #4321, I want a has_coe_to_fun so I can write grades x i to get the ith grade of x. I could instead declare a type alias there. Which approach makes more sense to you?

@eric-wieser Would that become much harder with coeff?

For polynomial I definitely wouldn't want the has_coe_to_fun. But for monoid_algebra I'm not so sure... maybe it can work. Still, I'm inclined to say that we shouldn't abuse that class, and only use if for things that we really think of as functions.

What ever option we choose, it would be good to have simp-lemmas that state how coe/coeff interacts with 0/1/+/etc.

@eric-wieser
Copy link
Member Author

eric-wieser commented Oct 3, 2020

@eric-wieser Would that become much harder with coeff?

No, what I'd probably do is add a grade_result := add_monoid_algebra alias in that file and define has_coe_to_fun on that.

If we don't want has_coe_to_fun in this PR, then is

lemma mul_apply (f g : monoid_algebra k G) (x : G) :
(f * g) x = (f.sum $ λa₁ b₁, g.sum $ λa₂ b₂, if a₁ * a₂ = x then b₁ * b₂ else 0) :=
begin
rw [mul_def],
simp only [finsupp.sum_apply, single_apply],
end
supposed to be a private lemma? It's not useable anywhere that doesn't have has_coe_to_fun, either via this PR or marking it as reducible.

@jcommelin
Copy link
Member

Well, we either want coeff or has_coe_to_fun. Having neither seems very bad 😄
And so the mul_apply should be stated in terms of whatever we choose the API to be.

My intuition says: have coeff for a general (add_)monoid_algebra, and then do the has_coe_to_fun for the graded setting.

@eric-wieser
Copy link
Member Author

I have no intuition at all about monoid_algebra, other than that they appear to be a superset of the behavior I want for my grading - so will defer to your decision on this.

@semorrison
Copy link
Collaborator

You're absolutely right that mul_apply reveals some poor design (mine, I think!).

My preference is definitely to introduce coeff. However I'm not going to do that myself in the near future, and if given the choice of no change, vs adding the has_coe_to_fun as a stopgap, I'd add has_coe_to_fun.

Do you want to have a go at introducing coeff for monoid_algebra? If not, let's get this merged, and perhaps create an issue to track the desire to introduce coeff later.

@jcommelin jcommelin added awaiting-author A reviewer has asked the author a question or requested changes and removed awaiting-review The author would like community review of the PR labels Oct 5, 2020
@eric-wieser eric-wieser force-pushed the eric-wieser/monoid_algebra-tweaks branch from c31795f to 9f22aa6 Compare October 12, 2020 08:17
@jcommelin
Copy link
Member

@eric-wieser @semorrison What shall we do here?

@eric-wieser
Copy link
Member Author

I don't have any particular interest in developing monoid_algebra beyond what I need in #4321 - if the goal is to eventually add monoid_algebra.coeff, then that sounds like a refactor that should involve moving stuff from polynomial to monoid_algebra; something that should probably be done by someone who uses polynomial, not me.

As an argument for has_coe_to_fun - I quite like the model that monoid_algebra is "just finsupp but with convolutive multiplication". I suppose I could extract a conv_finsupp base class?

@eric-wieser
Copy link
Member Author

Alternatively, since my only motivation here is #4321, we could close this PR and merge the two line change here as part of that PR, to make the motivation clear from the commit log.

@jcommelin
Copy link
Member

@semorrison What do you think, shall we merge this for now (since mul_apply is already implicitly depending on this) and then refactor to use coeff at a later point?

@jcommelin jcommelin removed the awaiting-author A reviewer has asked the author a question or requested changes label Oct 19, 2020
@jcommelin jcommelin added the awaiting-review The author would like community review of the PR label Oct 19, 2020
@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 Oct 19, 2020
bors bot pushed a commit that referenced this pull request Oct 19, 2020
Also does the same for the additive version `semimodule k (add_monoid_algebra k G)`.
@bors
Copy link

bors bot commented Oct 20, 2020

Pull request successfully merged into master.

Build succeeded:

@bors bors bot changed the title feat(data/monoid_algebra): add missing has_coe_to_fun [Merged by Bors] - feat(data/monoid_algebra): add missing has_coe_to_fun Oct 20, 2020
@bors bors bot closed this Oct 20, 2020
@bors bors bot deleted the eric-wieser/monoid_algebra-tweaks branch October 20, 2020 01: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

4 participants