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(*): add a div/sub field to (add_)group(_with_zero) #5303

Closed
wants to merge 15 commits into from

Conversation

Vierkantor
Copy link
Collaborator

@Vierkantor Vierkantor commented Dec 10, 2020

This PR is intended to fix the following kind of diamonds:
Let foo X be a type with a ∀ X, has_div (foo X) instance but no ∀ X, has_inv (foo X), e.g. when foo X is a euclidean_domain. Suppose we also have an instance ∀ X [cromulent X], group_with_zero (foo X). Then the (/) coming from group_with_zero_has_div cannot be defeq to the (/) coming from foo.has_div.

As a consequence of making the has_div instances defeq, we can no longer assume that (div_eq_mul_inv a b : a / b = a * b⁻¹) = rfl for all groups. The previous preparation PR #5302 should have changed all places in mathlib that assumed defeqness, to rewrite explicitly instead.


@Vierkantor Vierkantor added WIP Work in progress blocked-by-other-PR This PR depends on another PR which is still in the queue. A bot manages this label via PR comment. labels Dec 10, 2020
@Vierkantor Vierkantor force-pushed the sub_div_monoid branch 2 times, most recently from 0f9f524 to fcaa620 Compare December 10, 2020 12:40
@github-actions github-actions bot added the merge-conflict Please `git merge origin/master` then a bot will remove this label. label Dec 10, 2020
bors bot pushed a commit that referenced this pull request Dec 11, 2020
#5302)

This PR prepares for a follow-up PR (#5303) that adds `div` and `sub` fields to (`add_`)`group`(`_with_zero`). That follow-up PR is intended to fix the following kind of diamonds:
Let `foo X` be a type with a `∀ X, has_div (foo X)` instance but no `∀ X, has_inv (foo X)`, e.g. when `foo X` is a `euclidean_domain`. Suppose we also have an instance `∀ X [cromulent X], group_with_zero (foo X)`. Then the `(/)` coming from `group_with_zero_has_div` cannot be defeq to the `(/)` coming from `foo.has_div`.

As a consequence of making the `has_div` instances defeq, we can no longer assume that `(div_eq_mul_inv a b : a / b = a * b⁻¹) = rfl` for all groups. This preparation PR should have changed all places in mathlib that assumed defeqness, to rewrite explicitly instead.

Zulip thread: https://leanprover.zulipchat.com/#narrow/stream/113488-general/topic/.60div.60.20as.20a.20field.20in.20.60group(_with_zero).60



Co-authored-by: sgouezel <sebastien.gouezel@univ-rennes1.fr>
@github-actions github-actions bot removed the blocked-by-other-PR This PR depends on another PR which is still in the queue. A bot manages this label via PR comment. label Dec 11, 2020
@github-actions
Copy link

🎉 Great news! Looks like all the dependencies have been resolved:

💡 To add or remove a dependency please update this issue/PR description.

Brought to you by Dependent Issues (:robot: ). Happy coding!

@github-actions github-actions bot removed the merge-conflict Please `git merge origin/master` then a bot will remove this label. label Dec 11, 2020
@Vierkantor Vierkantor added awaiting-review The author would like community review of the PR and removed WIP Work in progress labels Dec 11, 2020
@Vierkantor
Copy link
Collaborator Author

There seem to be no obvious breakages in the low-level files, so let's see what CI thinks of the whole of mathlib...

@sgouezel
Copy link
Collaborator

Does not build yet

@sgouezel sgouezel 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 Dec 14, 2020
@Vierkantor
Copy link
Collaborator Author

Now it compiles successfully on my machine.

@Vierkantor Vierkantor added awaiting-review The author would like community review of the PR and removed awaiting-author A reviewer has asked the author a question or requested changes labels Dec 14, 2020
Copy link
Collaborator

@sgouezel sgouezel left a comment

Choose a reason for hiding this comment

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

It would be worth adding somehwere a comment explaining why you choose to have fields sub and div instead of using x + - y or x * y^{-1}, by mentioning the example that is a motivation for the PR.

src/algebra/group_with_zero/basic.lean Outdated Show resolved Hide resolved
@Vierkantor
Copy link
Collaborator Author

It would be worth adding somehwere a comment explaining why you choose to have fields sub and div instead of using x + - y or x * y^{-1}, by mentioning the example that is a motivation for the PR.

I have added this as a comment on the declarations of div_inv_monoid/sub_neg_monoid.

This PR is intended to fix the following kind of diamonds:
Let `foo X` be a type with a `∀ X, has_div (foo X)` instance but no `∀ X, has_inv (foo X)`,
   e.g. when `foo X` is a `euclidean_domain`.
Suppose we also have an instance `∀ X [cromulent X], group_with_zero (foo X)`.
Then the `(/)` coming from `group_with_zero_has_div` cannot be defeq to the `(/)` coming from `foo.has_div`.

As a consequence of making the `has_div` instances defeq, we can no longer
assume that `(div_eq_mul_inv a b : a / b = a * b⁻¹) = rfl` for all groups.
The previous preparation commit should have changed all places in mathlib
that assumed defeqness, to rewrite explicitly instead.
(I could have sworn the line was just short enough; I cannot count apparently)
The elaborator managed to apply it by itself in a really messed-up way,
so it looked like `norm_num` was unfolding stuff badly when really it was the
elaborator a few lines previously
@jcommelin
Copy link
Member

That was an epic refactor. Thanks a lot! 🎉

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 Dec 16, 2020
bors bot pushed a commit that referenced this pull request Dec 16, 2020
…5303)

This PR is intended to fix the following kind of diamonds:
Let `foo X` be a type with a `∀ X, has_div (foo X)` instance but no `∀ X, has_inv (foo X)`, e.g. when `foo X` is a `euclidean_domain`. Suppose we also have an instance `∀ X [cromulent X], group_with_zero (foo X)`. Then the `(/)` coming from `group_with_zero_has_div` cannot be defeq to the `(/)` coming from `foo.has_div`.

As a consequence of making the `has_div` instances defeq, we can no longer assume that `(div_eq_mul_inv a b : a / b = a * b⁻¹) = rfl` for all groups. The previous preparation PR #5302 should have changed all places in mathlib that assumed defeqness, to rewrite explicitly instead.



Co-authored-by: Anne Baanen <Vierkantor@users.noreply.github.com>
@bors
Copy link

bors bot commented Dec 16, 2020

Pull request successfully merged into master.

Build succeeded:

@bors bors bot changed the title chore(*): add a div/sub field to (add_)group(_with_zero) [Merged by Bors] - chore(*): add a div/sub field to (add_)group(_with_zero) Dec 16, 2020
@bors bors bot closed this Dec 16, 2020
@bors bors bot deleted the sub_div_monoid branch December 16, 2020 18:35
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