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/*set): some finite sets lemmas #7037

Closed
wants to merge 4 commits into from

Conversation

PatrickMassot
Copy link
Member

@PatrickMassot PatrickMassot added the awaiting-review The author would like community review of the PR label Apr 5, 2021
@@ -34,6 +34,16 @@ end function
protected def equiv.to_embedding {α : Sort u} {β : Sort v} (f : α ≃ β) : α ↪ β :=
⟨f, f.injective⟩

/-- Given an equivalence to a coerced set `↥s`, produce an embedding to the elements of that set. -/
@[simps]
def equiv.as_embedding {α β : Sort*} {s : set β} (e : α ≃ ↥s) : α ↪ β :=
Copy link
Member

Choose a reason for hiding this comment

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

This name is a little similar to equiv.to_embedding.

Also, perhaps we should generalize ↥s to subtype p.

Copy link
Member

Choose a reason for hiding this comment

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

That is,

/-- Given an equivalence to a subtype, produce an embedding to the elements of enclosing type. -/
@[simps]
def equiv.embedding_of_subtype {α β : Type*} {p : β → Prop} (e : α ≃ subtype p) : α ↪ β :=
e.to_embedding.trans $ function.embedding.subtype p

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 know the name is similar, but I don't have a better idea. equiv.to_embedding has the right to use this name, and I didn't want to use a very long name. Suggestions are welcome.

Copy link
Member

Choose a reason for hiding this comment

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

You're not a fan of the name equiv.embedding_of_subtype then?

Copy link
Member

@jcommelin jcommelin Apr 6, 2021

Choose a reason for hiding this comment

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

It should be embedding_of_equiv_subtype then, right? I think equiv.as_embbeding is fine.
Also, sorry for kicking this on the queue. I missed that this conversation hadn't come to a conclusion yet.

Copy link
Member

Choose a reason for hiding this comment

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

I figured the word equiv was redundant, hence dropping it from embedding_of_equiv_subtype.

Getting subtype in the name will probably make this more discoverable.

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.

Thanks 🎉

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 Apr 6, 2021
@jcommelin
Copy link
Member

bors r-

(Still a conversation ongoing.)

@bors
Copy link

bors bot commented Apr 6, 2021

Canceled.

Comment on lines +37 to +41
/-- Given an equivalence to a subtype, produce an embedding to the elements of the corresponding
set. -/
@[simps]
def equiv.as_embedding {α β : Sort*} {p : β → Prop} (e : α ≃ subtype p) : α ↪ β :=
⟨coe ∘ e, subtype.coe_injective.comp e.injective⟩
Copy link
Member

Choose a reason for hiding this comment

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

Github marked #7037 (comment) as outdated, which discusses this name.

Copy link
Member

Choose a reason for hiding this comment

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

Unrelated to the name conversation, we have some API to construct this:

Suggested change
/-- Given an equivalence to a subtype, produce an embedding to the elements of the corresponding
set. -/
@[simps]
def equiv.as_embedding {α β : Sort*} {p : β → Prop} (e : α ≃ subtype p) : α ↪ β :=
⟨coe ∘ e, subtype.coe_injective.comp e.injective⟩
/-- Given an equivalence to a subtype, produce an embedding to the elements of the corresponding
set. -/
@[simps]
def equiv.as_embedding {α β : Sort*} {p : β → Prop} (e : α ≃ subtype p) : α ↪ β :=
e.to_embedding.trans $ function.embedding.subtype p

which may or may not break the proof below.

@PatrickMassot
Copy link
Member Author

I think it would be much faster if @eric-wieser could directly push to this branch. I really don't care at all, I simply want this in order to do interesting linear algebra with them. Any variation will be good enough for me.

@eric-wieser
Copy link
Member

I didn't want to tread on your toes, and was waiting for a second opinion on the name. @jcommelin, if you pick a name I'll happily push to the branch

@jcommelin
Copy link
Member

I understand the benefit of having subtype in the name. But I still find the proposal unnatural. It sounds like we are assuming some (x : subtype p).
So I suggest we just leave it as it is.

@jcommelin
Copy link
Member

Thanks 🎉

bors merge

bors bot pushed a commit that referenced this pull request Apr 6, 2021
Co-authored-by: Bhavik Mehta, Kevin Buzzard, Anne Baanen, Eric Rodriguez
@bors
Copy link

bors bot commented Apr 7, 2021

Build failed (retrying...):

bors bot pushed a commit that referenced this pull request Apr 7, 2021
Co-authored-by: Bhavik Mehta, Kevin Buzzard, Anne Baanen, Eric Rodriguez
@bors
Copy link

bors bot commented Apr 7, 2021

Pull request successfully merged into master.

Build succeeded:

@bors bors bot changed the title feat(data/*set): some finite sets lemmas [Merged by Bors] - feat(data/*set): some finite sets lemmas Apr 7, 2021
@bors bors bot closed this Apr 7, 2021
@bors bors bot deleted the finite-stuff branch April 7, 2021 10:30
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

3 participants