Skip to content
This repository has been archived by the owner on Jul 24, 2024. It is now read-only.

chore(data/multiset): move has_repr into its own file to remove data.string.basic import #18075

Closed
wants to merge 5 commits into from

Conversation

ChrisHughes24
Copy link
Member


I did it like this instead of moving the deifnition of le to string.defs because the repr instance also depends on some properties of the ordering to prove it is well defined on multisets.

Open in Gitpod

@ChrisHughes24 ChrisHughes24 added the awaiting-CI The author would like to see what CI has to say before doing more work. label Jan 6, 2023
@ChrisHughes24 ChrisHughes24 added awaiting-review The author would like community review of the PR and removed awaiting-CI The author would like to see what CI has to say before doing more work. labels Jan 6, 2023
@jcommelin
Copy link
Member

Thanks 🎉

If CI passes, please remove the label awaiting-CI and merge this yourself, by adding a comment bors r+.

bors d+

@bors
Copy link

bors bot commented Jan 6, 2023

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

@jcommelin jcommelin added awaiting-CI The author would like to see what CI has to say before doing more work. and removed awaiting-review The author would like community review of the PR labels Jan 6, 2023
@github-actions github-actions bot added the delegated The PR author may merge after reviewing final suggestions. label Jan 6, 2023
@ChrisHughes24
Copy link
Member Author

I built this locally

bors r+

@bors
Copy link

bors bot commented Jan 6, 2023

👎 Rejected by label

@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 Jan 6, 2023
@ChrisHughes24 ChrisHughes24 removed ready-to-merge All that is left is for bors to build and merge this PR. (Remember you need to say `bors r+`.) awaiting-CI The author would like to see what CI has to say before doing more work. labels Jan 6, 2023
@ChrisHughes24
Copy link
Member Author

bors r+

@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 Jan 6, 2023
@ChrisHughes24
Copy link
Member Author

Maybe it's better to make these into meta instances

bors r-

@bors
Copy link

bors bot commented Jan 6, 2023

Canceled.

@eric-wieser eric-wieser removed 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 Jan 8, 2023
@eric-wieser
Copy link
Member

eric-wieser commented Jan 8, 2023

I think a better approach would be to hide the meta behind a typeclass instance: something like:

import logic.embedding.basic
import data.prod.lex
import data.pi.lex
import logic.equiv.basic
import data.string.basic
import data.multiset.sort

/-- When printing collections of `α`, sort via a representation in `αo` -/
class has_repr_order_key (α : Type*) (αo : out_param Type*) [linear_order αo] :=
(to_key : α ↪ αo)

instance linear_order.to_has_repr_order_key {α} [linear_order α] : has_repr_order_key α α :=
⟨function.embedding.refl _⟩

instance prod.to_has_repr_order_key {α β αo βo}
  [linear_order αo] [linear_order βo] [has_repr_order_key α αo] [has_repr_order_key β βo] :
  has_repr_order_key (α × β) (αo ×ₗ βo) :=
⟨(has_repr_order_key.to_key.prod_map has_repr_order_key.to_key).trans to_lex.to_embedding⟩

-- TODO: `pi` order

instance (α αo : Type*) [linear_order αo] [has_repr_order_key α αo]:
  is_linear_order α ((≤) on has_repr_order_key.to_key) :=
@has_le.le.is_linear_order _ $
  linear_order.lift' (has_repr_order_key.to_key : α → αo) has_repr_order_key.to_key.injective

/-- fallback instance using string sorting -/
@[priority 100, instance]
meta instance has_repr.to_has_repr_order_key {α} [has_repr α] :
  has_repr_order_key α string :=
⟨⟨repr, sorry⟩⟩  -- We can cheat to make this not a sorry since this is meta, right?

#eval ({1, (1, 5), (5, 1), 11} : multiset (ℕ × ℕ)).sort ((≤) on has_repr_order_key.to_key)

@jcommelin jcommelin added the awaiting-author A reviewer has asked the author a question or requested changes label Jan 9, 2023
@eric-wieser
Copy link
Member

I've created #18166 to track my suggestion above, we can always revisit it in Mathlib4 after the port.

I assume this PR should be closed in light of your new PR, #18163?

@ChrisHughes24 ChrisHughes24 deleted the finset-multiset-repr branch January 13, 2023 18:55
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
awaiting-author A reviewer has asked the author a question or requested changes delegated The PR author may merge after reviewing final suggestions.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants