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/list/destutter): add list.destutter to remove chained duplicates #11934

Closed
wants to merge 19 commits into from

Conversation

ericrbg
Copy link
Collaborator

@ericrbg ericrbg commented Feb 9, 2022


Open in Gitpod

@ericrbg ericrbg added the awaiting-review The author would like community review of the PR label Feb 9, 2022
Copy link
Member

@eric-wieser eric-wieser left a comment

Choose a reason for hiding this comment

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

Am I blind, or are you missing the lemma that the list with no pairwise duplicates satisfies list.chain' ne?

On that note, should this be called chain'_dedup, since list.pairwise isn't about adjacency?

@ericrbg
Copy link
Collaborator Author

ericrbg commented Feb 10, 2022

I like chain'_dedup. I had no real good idea on what to call it. I'll add that proof! Do you have any other ideas what other lemmas to add, maybe that it's idempotent & that if list.chain ne' then it's the identity?

@ericrbg
Copy link
Collaborator Author

ericrbg commented Feb 10, 2022

oh, I now see why the naming is as it is. I guess I can name the aux chain_dedup and the non-aux chain'_dedup; I'd argue that these should be the other way round, but ¯\_(ツ)_/¯

@ericrbg
Copy link
Collaborator Author

ericrbg commented Feb 10, 2022

well, that's a bug in the buildscript! we can't have 's in names of files

@eric-wieser
Copy link
Member

Yes, that's #11001

@ericrbg
Copy link
Collaborator Author

ericrbg commented Feb 10, 2022

it's a separate one from that; https://github.com/leanprover-community/mathlib/runs/5140357750?check_suite_focus=true

it's to do with xargs, not Lean itself

@eric-wieser eric-wieser changed the title feat(data/list/pairwise_dup): remove lists' pairwise duplicates. feat(data/list/chain_dup): remove lists' chained duplicates Feb 10, 2022
@eric-wieser eric-wieser changed the title feat(data/list/chain_dup): remove lists' chained duplicates feat(data/list/chain_dup): add list.chain_dedup' to remove chained duplicates Feb 10, 2022
@eric-wieser
Copy link
Member

I pushed a couple more golfs, and a l.chain'_dedup = l ↔ l.chain' ne simp lemma

Copy link
Member

@eric-wieser eric-wieser left a comment

Choose a reason for hiding this comment

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

LGTM, let's let a fresh pair of eyes confirm.

If you're feeling brave, the following lemmas would probably be useful:

  • something about concat (l ++ [a])
  • something about append
  • l.reverse.chain'_dedup = l.chain'_dedup.reverse

Comment on lines 33 to 36
/-- Removes all adjacent duplicates in `a :: l`. -/
def chain_dedup : α → list α → list α
| a [] := []
| a (h :: l) := if a = h then chain_dedup h l else h :: chain_dedup h l
Copy link
Member

@digama0 digama0 Feb 10, 2022

Choose a reason for hiding this comment

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

  1. This and chain_dedup' should be in data.list.defs. Also could there be a version that accepts a relation?
  2. The chain naming suggests that it is related to list.chain, but that definition takes a relation.
  3. The use of h :: chain_dedup h l looks like a typo. Shouldn't it be a ::? This will always discard a even if a :: l has no duplicates.

Copy link
Collaborator Author

@ericrbg ericrbg Feb 10, 2022

Choose a reason for hiding this comment

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

I thought about the relation generalisation, but then if !a R b, which one would I discard?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

and on 3, it seems to break all my proofs in ways that make it seem genuinely different from what I'd expect - if I'm honest, I focused mainly on chain'_dedup, so I didn't pay as close attention as to how exactly to implement chain_dedup without it. Do you think the base case should be | a [] := [a]? That seems reasonable, now that I think about it a little

Copy link
Member

Choose a reason for hiding this comment

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

Discarding the later entry in the list is probably the right choice - the description then becomes "greedily take items from the list (in order) discarding any that do not satisfy R last_accepted candidate", and the current definitions are just the special case when R = ne

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm busy for the next while, but when I next get a chance I'll try work on that generalisation.

Copy link
Member

Choose a reason for hiding this comment

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

Note that with that generalization, this is the chain version of list.pw_filter

src/data/list/chain_dup.lean Outdated Show resolved Hide resolved
@ericrbg ericrbg 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 Feb 14, 2022
@ericrbg ericrbg 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 Feb 15, 2022
@ericrbg ericrbg changed the title feat(data/list/chain_dup): add list.chain_dedup' to remove chained duplicates feat(data/list/chain_dup): add list.destutter' to remove chained duplicates Feb 15, 2022
@ericrbg ericrbg changed the title feat(data/list/chain_dup): add list.destutter' to remove chained duplicates feat(data/list/chain_dup): add list.destutter to remove chained duplicates Feb 15, 2022
@ericrbg ericrbg changed the title feat(data/list/chain_dup): add list.destutter to remove chained duplicates feat(data/list/destutter): add list.destutter to remove chained duplicates Feb 21, 2022
src/data/list/defs.lean Outdated Show resolved Hide resolved
@jcommelin
Copy link
Member

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 Mar 3, 2022
bors bot pushed a commit that referenced this pull request Mar 3, 2022
…licates (#11934)

Co-authored-by: Eric Wieser <wieser.eric@gmail.com>
@bors
Copy link

bors bot commented Mar 3, 2022

Pull request successfully merged into master.

Build succeeded:

@bors bors bot changed the title feat(data/list/destutter): add list.destutter to remove chained duplicates [Merged by Bors] - feat(data/list/destutter): add list.destutter to remove chained duplicates Mar 3, 2022
@bors bors bot closed this Mar 3, 2022
@bors bors bot deleted the pairwise_dups branch March 3, 2022 12:56
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

6 participants