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/sym2): add the universal property, and make sym2.is_diag ⟦(x, y)⟧ ↔ x = y true definitionally #8358

Closed
wants to merge 6 commits into from

Conversation

eric-wieser
Copy link
Member


Open in Gitpod

@eric-wieser eric-wieser added the awaiting-review The author would like community review of the PR label Jul 18, 2021
prod.rec_on z $ λ _ _, is_diag_iff_eq

@[simp]
lemma diag_is_diag (a : α) : is_diag (diag a) :=
Copy link
Collaborator

@pechersky pechersky Jul 18, 2021

Choose a reason for hiding this comment

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

Suggested change
lemma diag_is_diag (a : α) : is_diag (diag a) :=
lemma is_diag_diag (a : α) : is_diag (diag a) :=

And the appropriate edits below

Copy link
Member Author

Choose a reason for hiding this comment

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

This name is the same as it was before this PR, but I suppose I could creep the scope to fix the name.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I get confused about the name to give compositions like this, and I get even more confused when dot notation is involved. If the conclusion were written as (diag a).is_diag (which it could be), would it still be is_diag_diag?

I think I'm guilty putting is_foo predicates at the ends of lemmas elsewhere, too, which is why I ask.

Copy link
Member Author

Choose a reason for hiding this comment

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

I made a Zulip thread.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've also reverted the change - if it's contentious, then renaming it is outside the scope of this PR.

`sym2.from_rel` is a more convenient spelling when working with `Prop`. -/
def lift {β : Sort*} :
{f : α → α → β // ∀ a₁ a₂, f a₁ a₂ = f a₂ a₁} ≃ (sym2 α → β) :=
{ to_fun := λ f, quotient.lift (λ a : α × α, (f : α → α → β) a.1 a.2) $
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you please make that to_fun a separate definition? We needed that generalisation of sym2.from_rel some time ago.

Copy link
Member Author

Choose a reason for hiding this comment

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

What would be the point of the separate definition? If you use lift it unfolds to this anyway.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Well I guess it's easier to have a definition without mentioning a subtype.

def from_fun {β : Sort*} {f : α → α → β} (comm : ∀ ⦃x y⦄, f x y = f y x) :
  sym2 α → β :=
quotient.lift (uncurry f) (by { rintro ⟨x, y⟩ _ ⟨_, _⟩, {refl }, unfold uncurry, rw comm })

Copy link
Member Author

@eric-wieser eric-wieser Jul 18, 2021

Choose a reason for hiding this comment

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

That def results in {β : Type*} since uncurry isn't defined on sorts:

#check @from_fun
-- Π {α : Type u_1} {β : Type u_2} {f : α → α → β}, (∀ ⦃x y : α⦄, f x y = f y x) → sym2 α → β

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh that's annoying... There's no real reason for it, right?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Wait, what? The original definition of from_rel uses function.uncurry r. What is this witchcraft?

Copy link
Member Author

Choose a reason for hiding this comment

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

Looking again, it looks like there's no point in letting lift work on Sort anyway, no one every needs a subtype of a proof, and quotient.induction works just fine for the cases when Sort would apply.

src/data/sym2.lean Outdated Show resolved Hide resolved
`sym2.from_rel` instead. -/
def lift {β : Type*} : {f : α → α → β // ∀ a₁ a₂, f a₁ a₂ = f a₂ a₁} ≃ (sym2 α → β) :=
{ to_fun := λ f, quotient.lift (uncurry ↑f) $ by { rintro _ _ ⟨⟩, exacts [rfl, f.prop _ _] },
inv_fun := λ F, ⟨λ a₁ a₂, F ⟦(a₁, a₂)⟧, λ a₁ a₂, congr_arg F eq_swap⟩,
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we want to maintain the point-free style of to_fun, then inv_fun could be

  inv_fun := λ F, ⟨curry (F ∘ quotient.mk), λ a₁ a₂, congr_arg F eq_swap⟩,

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, why not. I added a lemma that unfolds it to my version, but it's nice to see the composition in lift.inv_fun, since there almost always is one.

Copy link
Collaborator

@kmill kmill left a comment

Choose a reason for hiding this comment

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

sym2.lift should have existed long ago -- thanks!

Since there's no consensus, lets leave renaming this for another time.

Also adds a docstring and a lemma
Copy link
Collaborator

@b-mehta b-mehta left a comment

Choose a reason for hiding this comment

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

Looks good to me! I don't have a strong opinion on the is_diag_diag naming choice, so I'll let @eric-wieser decide what to do there.

bors d+

@bors
Copy link

bors bot commented Jul 20, 2021

✌️ eric-wieser can now approve this pull request. To approve and merge a pull request, simply reply with bors r+. More detailed instructions are available here.

@github-actions github-actions bot added delegated The PR author may merge after reviewing final suggestions. and removed awaiting-review The author would like community review of the PR labels Jul 20, 2021
@eric-wieser
Copy link
Member Author

bors r+

I've left the name untouched, someone else can do a wider rename in a separate PR.

bors bot pushed a commit that referenced this pull request Jul 20, 2021
@github-actions github-actions bot added the ready-to-merge All that is left is for bors to build and merge this PR. (Remember you need to say `bors r+`.) label Jul 20, 2021
@bors
Copy link

bors bot commented Jul 20, 2021

Build failed:

@eric-wieser
Copy link
Member Author

bors r+

bors bot pushed a commit that referenced this pull request Jul 20, 2021
@bors
Copy link

bors bot commented Jul 20, 2021

Pull request successfully merged into master.

Build succeeded:

@bors bors bot changed the title feat(data/sym2): add the universal property, and make sym2.is_diag ⟦(x, y)⟧ ↔ x = y true definitionally [Merged by Bors] - feat(data/sym2): add the universal property, and make sym2.is_diag ⟦(x, y)⟧ ↔ x = y true definitionally Jul 20, 2021
@bors bors bot closed this Jul 20, 2021
@bors bors bot deleted the eric-wieser/sym2.lift branch July 20, 2021 18:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
delegated The PR author may merge after reviewing final suggestions. 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