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/dfinsupp): Relax requirements of semimodule conversion to add_comm_monoid #3490

Closed
wants to merge 9 commits into from

Conversation

eric-wieser
Copy link
Member

@eric-wieser eric-wieser commented Jul 21, 2020

The extra _s required to make this still build are unfortunate, but hopefully someone else can work out how to remove them in a later PR.


Continues the work in #2848, cc @sgouezel

@eric-wieser eric-wieser marked this pull request as ready for review July 21, 2020 14:01
@eric-wieser eric-wieser added the awaiting-review The author would like community review of the PR label Jul 21, 2020
@semorrison
Copy link
Collaborator

Isn't the problem here that direct_sum itself here is still all written in terms of add_comm_group, rather than add_comm_monoid?

Perhaps we should refactor that first before attempting this change.

@semorrison
Copy link
Collaborator

(To be honest, I'm disinclined to do this myself because I just want to rip out everything we have on direct sums and replace it with stuff using the category theory library, so we don't have to go around writing boilerplate about how the projection maps have is_add_group_hom instances, etc. Probably I'd just be replacing it with different boilerplate, but ...)

@semorrison
Copy link
Collaborator

Oh, I see, I'm slow, that's in #3487. Could you explain why that PR is blocked on this one? I would have guessed it was the other way round.

@eric-wieser
Copy link
Member Author

It's blocked primarily because I don't want to bite off too much in a single PR, and this only had two downstream proofs that were affected. The other PR looked far harder to resolve, and conflicts with #3473 anyway.

@semorrison
Copy link
Collaborator

I guess I still don't understand why #3487 is blocked on this. Can't we start generalizing that file without touching anything here?

@semorrison
Copy link
Collaborator

(Sorry about the slow responses on these PRs. I'll try to check back sooner.)

@eric-wieser eric-wieser force-pushed the eric-wieser/relax-dfinsupp.to_semimodule branch from 7d89257 to 5c74907 Compare July 24, 2020 07:19
@eric-wieser
Copy link
Member Author

Blocked is a strong word. It seemed this was easier to get past the finish line, and the PRs were likely to conflict.

@semorrison
Copy link
Collaborator

semorrison commented Jul 25, 2020

I'm going to mark this back as WIP. My understanding is that:

  1. this change is essentially a regression: we shouldn't be making this change that introduces the need for lots of new underscores, without fixing the underlying problem.
  2. You have another overlapping PR open, [Merged by Bors] - feat(algebra/direct_sum*): relax a lot of constraints to add_comm_monoid #3537, which is also working on a solution.

Once you've sorted out which approach is best, could you mark that one as request-review again? (And/or close other PRs as appropriate.)

@semorrison semorrison added WIP Work in progress and removed awaiting-review The author would like community review of the PR labels Jul 25, 2020
@github-actions github-actions bot added merge-conflict Please `git merge origin/master` then a bot will remove this label. and removed merge-conflict Please `git merge origin/master` then a bot will remove this label. labels Aug 13, 2020
@github-actions github-actions bot removed the merge-conflict Please `git merge origin/master` then a bot will remove this label. label Sep 11, 2020
@github-actions github-actions bot added the merge-conflict Please `git merge origin/master` then a bot will remove this label. label Sep 28, 2020
@eric-wieser eric-wieser force-pushed the eric-wieser/relax-dfinsupp.to_semimodule branch from 6d605f7 to 0ef30d2 Compare October 23, 2020 10:44
@github-actions github-actions bot removed the merge-conflict Please `git merge origin/master` then a bot will remove this label. label Oct 23, 2020
@eric-wieser
Copy link
Member Author

eric-wieser commented Oct 23, 2020

this change is essentially a regression: we shouldn't be making this change that introduces the need for lots of new underscores, without fixing the underlying problem.

My argument is that we have a pile of code that assumes add_comm_group and should assume add_comm_monoid. The only options are:

  • Fix the whole pile
  • Start from the bottom of the pile, and introduce temporary ugly underscores as we move up the pile

I don't see why the second option is a problem here. Can't we do this incrementally?

(I've found myself needing this specific change again, and I don't want to have to revive the other other PRs about direct_sum which I don't care about right now)

@eric-wieser eric-wieser added awaiting-review The author would like community review of the PR and removed WIP Work in progress labels Oct 23, 2020
Copy link
Collaborator

@Vierkantor Vierkantor left a comment

Choose a reason for hiding this comment

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

Generalizing to add_comm_monoid is a good idea and few more underscores doesn't hurt the readability too much IMO, so I'm in favour of this PR.

src/data/dfinsupp.lean Outdated Show resolved Hide resolved
src/algebra/lie/basic.lean Outdated Show resolved Hide resolved
src/algebra/direct_limit.lean Outdated Show resolved Hide resolved
Co-authored-by: Anne Baanen <Vierkantor@users.noreply.github.com>
@github-actions github-actions bot added the merge-conflict Please `git merge origin/master` then a bot will remove this label. label Oct 28, 2020
@github-actions github-actions bot removed the merge-conflict Please `git merge origin/master` then a bot will remove this label. label Oct 29, 2020
@digama0
Copy link
Member

digama0 commented Oct 29, 2020

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 29, 2020
bors bot pushed a commit that referenced this pull request Oct 29, 2020
…dd_comm_monoid (#3490)

The extra `_`s required to make this still build are unfortunate, but hopefully someone else can work out how to remove them in a later PR.



Co-authored-by: Mario Carneiro <di.gama@gmail.com>
@bors
Copy link

bors bot commented Oct 29, 2020

Pull request successfully merged into master.

Build succeeded:

@bors bors bot changed the title feat(data/dfinsupp): Relax requirements of semimodule conversion to add_comm_monoid [Merged by Bors] - feat(data/dfinsupp): Relax requirements of semimodule conversion to add_comm_monoid Oct 29, 2020
@bors bors bot closed this Oct 29, 2020
@bors bors bot deleted the eric-wieser/relax-dfinsupp.to_semimodule branch October 29, 2020 14:18
lecopivo pushed a commit to lecopivo/mathlib that referenced this pull request Oct 31, 2020
…dd_comm_monoid (leanprover-community#3490)

The extra `_`s required to make this still build are unfortunate, but hopefully someone else can work out how to remove them in a later PR.



Co-authored-by: Mario Carneiro <di.gama@gmail.com>
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