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] - refactor: remove Sym2's global Prod setoid instance, use s(x, y) notation for unordered pairs #8729

Closed
wants to merge 5 commits into from

Conversation

kmill
Copy link
Contributor

@kmill kmill commented Nov 30, 2023

The Sym2 type used a global setoid instance on α × α so that ⟦(x, y)⟧ could stand for an unordered pair using standard Quotient syntax. This commit refactors Sym2 to not use Quotient and instead use its own s(x, y) notation. One benefit to this is that this notation produces a term with type Sym2 rather than Quotient.

The Fintype instance for Sym2 is in Mathlib.Data.Finset.Sym. We switch from using the one for Quotient because it does not require DecidableEq.


Open in Gitpod

@kmill kmill added the awaiting-review The author would like community review of the PR label Nov 30, 2023
Comment on lines -103 to +102
def Sym2 (α : Type u) :=
Quotient (Sym2.Rel.setoid α)
def Sym2 (α : Type u) := Quot (Sym2.Rel α)
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand what the gain of moving from Quotient to Quot is here; it looks like it just makes less API available...

(I agree with the move of de-instancing the Setoid instance and the new notation)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I strongly believe that any type defined using a quotient needs to recapitulate part of the quotient API so that it can ensure that there is no API leak.

The big problem with using the Quotient or Quot APIs on a type that happens to be defined using them is that (1) they change the type of the term from (for example) Sym2 to Quotient, (2) they use the constructor Quotient.mk instead of Sym2.mk (again, changing the type), (3) they use non-informative Setoid.r for the relation rather than Sym2.Rel. With that last point, it's hard writing these Quotient relation terms yourself when there's no global Setoid instance.

I am not sure what the Quotient API gives us that the Quot API couldn't -- the main one is Quotient.eq, but in the long run I'm imagining quotients would be more convenient if Quot had lemmas that could take a class version of Equivalence. That way relations wouldn't unnecessarily get turned into their Setoid-bundled forms.

One other reason I switched from Quotient to Quot is to disable the ⟦(x, y)⟧ notation.

You'll notice that in practice we can still use Quotient API by passing in a setoid. There's even a place where I temporarily enable the Setoid instance to use it. Other places I switched to Quot where it was more convenient, and I added Sym2 versions of recursors just to keep the types nice. (There's also the small point that the Quot recursors are missing @[elab_as_elim], which needs to be fixed in core.)

@eric-wieser eric-wieser 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 Nov 30, 2023
@kmill kmill 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 Nov 30, 2023
Mathlib/Data/Finset/Sym.lean Outdated Show resolved Hide resolved
Mathlib/Data/Finset/Sym.lean Outdated Show resolved Hide resolved
@@ -5,6 +5,7 @@ Authors: Kyle Miller
-/
import Mathlib.Combinatorics.SimpleGraph.Basic
import Mathlib.Algebra.BigOperators.Basic
import Mathlib.Data.Finset.Sym
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we need this extra import?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It brings in Sym2.instFintype

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm okay sure, but why do we suddenly need Sym2.instFintype here?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, is it because you can't use the Quotient instance anymore? Worth mentioning in the description.

Copy link
Contributor Author

@kmill kmill Dec 1, 2023

Choose a reason for hiding this comment

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

It's less a "can't" and more a "the Sym2.instFintype one is better". It's true that the Quotient instance would have to be restated using a inferInstanceAs instance to get it to apply though.

(I mentioned it in the other comment, but fleshing out the Quot API with a typeclass version of Equivalence would mean we could take these Quotient instances and make them be nicer-to-use Quot instances.)

Copy link
Member

Choose a reason for hiding this comment

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

Isn't that typeclass version HasEquiv?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, that's a notation typeclass.

I mean this:

class IsEquivalence {α : Type*} (r : α → α → Prop) : Prop where
  equivalence : Equivalence r

Co-authored-by: Yaël Dillies <yael.dillies@gmail.com>
@leanprover-community-mathlib4-bot leanprover-community-mathlib4-bot added the merge-conflict The PR has a merge conflict with master, and needs manual merging. label Dec 3, 2023
@leanprover-community-mathlib4-bot leanprover-community-mathlib4-bot removed the merge-conflict The PR has a merge conflict with master, and needs manual merging. label Dec 7, 2023
Copy link
Member

@jcommelin jcommelin left a comment

Choose a reason for hiding this comment

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

I'm going to kick this on the queue. The diff has net size close to zero. Outside the API file on sym2, it really cleans up some type-mess. The file on sym2 itself now duplicates some Quotient API. Maybe that can be done in a better way in the future. But that doesn't have to block this PR.

bors merge

@leanprover-community-mathlib4-bot leanprover-community-mathlib4-bot added ready-to-merge This PR has been sent to bors. and removed awaiting-review The author would like community review of the PR labels Dec 27, 2023
mathlib-bors bot pushed a commit that referenced this pull request Dec 27, 2023
…` notation for unordered pairs (#8729)

The `Sym2` type used a global setoid instance on `α × α` so that `⟦(x, y)⟧` could stand for an unordered pair using standard `Quotient` syntax. This commit refactors `Sym2` to not use `Quotient` and instead use its own `s(x, y)` notation. One benefit to this is that this notation produces a term with type `Sym2` rather than `Quotient`.

The `Fintype` instance for `Sym2` is in `Mathlib.Data.Finset.Sym`. We switch from using the one for `Quotient` because it does not require `DecidableEq`.
@mathlib-bors
Copy link

mathlib-bors bot commented Dec 27, 2023

Pull request successfully merged into master.

Build succeeded:

@mathlib-bors mathlib-bors bot changed the title refactor: remove Sym2's global Prod setoid instance, use s(x, y) notation for unordered pairs [Merged by Bors] - refactor: remove Sym2's global Prod setoid instance, use s(x, y) notation for unordered pairs Dec 27, 2023
@mathlib-bors mathlib-bors bot closed this Dec 27, 2023
@mathlib-bors mathlib-bors bot deleted the kmill_sym2_notation_refactor branch December 27, 2023 06:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge This PR has been sent to bors.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants