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

feat(Data/Multiset/Order): add Dershowitz-Manna Ordering and Theorem #14411

Open
wants to merge 34 commits into
base: master
Choose a base branch
from

Conversation

haitian-yuki
Copy link
Collaborator

@haitian-yuki haitian-yuki commented Jul 4, 2024

@github-actions github-actions bot added the new-contributor This PR was made by a contributor with at most 5 merged PRs. Welcome to the community! label Jul 4, 2024
Copy link

github-actions bot commented Jul 4, 2024

PR summary dbd9cbbc82

Import changes for modified files

No significant changes to the import graph

Import changes for all files
Files Import difference
Mathlib.Data.Multiset.DershowitzManna (new file) 390

Declarations diff

+ IsDershowitzMannaLT
+ IsDershowitzMannaLT.trans
+ instWellFoundedisDershowitzMannaLT
+ wellFounded_isDershowitzMannaLT

You can run this locally as follows
## summary with just the declaration names:
./scripts/declarations_diff.sh <optional_commit>

## more verbose report:
./scripts/declarations_diff.sh long <optional_commit>

The doc-module for script/declarations_diff.sh contains some details about this script.


No changes to technical debt.

You can run this locally as

./scripts/technical-debt-metrics.sh pr_summary
  • The relative value is the weighted sum of the differences with weight given by the inverse of the current value of the statistic.
  • The absolute value is the relative value divided by the total sum of the inverses of the current values (i.e. the weighted average of the differences).

@riccardobrasca
Copy link
Member

Can you please mark this PR WIP or awaiting-review? Thanks!

@riccardobrasca riccardobrasca added the awaiting-author A reviewer has asked the author a question or requested changes label Jul 4, 2024
Copy link
Collaborator

@Rida-Hamadani Rida-Hamadani left a comment

Choose a reason for hiding this comment

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

Thank you! Some comments are general and need to be applied in other places in PR (instead of just committing the suggested portions).

Mathlib/Data/Multiset/Order.lean Outdated Show resolved Hide resolved
Mathlib/Data/Multiset/Order.lean Outdated Show resolved Hide resolved
Mathlib/Data/Multiset/Order.lean Outdated Show resolved Hide resolved
Mathlib/Data/Multiset/Order.lean Outdated Show resolved Hide resolved
Mathlib/Data/Multiset/Order.lean Outdated Show resolved Hide resolved
Mathlib/Data/Multiset/Order.lean Outdated Show resolved Hide resolved
Mathlib/Data/Multiset/Order.lean Outdated Show resolved Hide resolved
Mathlib/Data/Multiset/Order.lean Outdated Show resolved Hide resolved
Mathlib/Data/Multiset/Order.lean Outdated Show resolved Hide resolved
Mathlib/Data/Multiset/Order.lean Outdated Show resolved Hide resolved
@haitian-yuki haitian-yuki removed the awaiting-author A reviewer has asked the author a question or requested changes label Jul 10, 2024
@kim-em
Copy link
Contributor

kim-em commented Jul 17, 2024

Obviously my review is far from complete, but I'm not an expert here so hopefully someone else will come along, but these will give you something to start with.

@kim-em kim-em added the awaiting-author A reviewer has asked the author a question or requested changes label Jul 18, 2024
@grunweg grunweg added the t-data Data (lists, quotients, numbers, etc) label Aug 15, 2024
@leanprover-community-mathlib4-bot leanprover-community-mathlib4-bot added the blocked-by-other-PR This PR depends on another PR to Mathlib (this label is automatically managed by a bot) label Aug 26, 2024
@mathlib4-dependent-issues-bot mathlib4-dependent-issues-bot removed the blocked-by-other-PR This PR depends on another PR to Mathlib (this label is automatically managed by a bot) label Sep 19, 2024
@mathlib4-dependent-issues-bot
Copy link
Collaborator

This PR/issue depends on:

@YaelDillies
Copy link
Collaborator

I mean replacing def/lemma by private def/private lemma

Mathlib/Data/Multiset/DershowitzManna.lean Outdated Show resolved Hide resolved
Mathlib/Data/Multiset/DershowitzManna.lean Outdated Show resolved Hide resolved
Mathlib/Data/Multiset/DershowitzManna.lean Outdated Show resolved Hide resolved
@haitian-yuki haitian-yuki removed the awaiting-author A reviewer has asked the author a question or requested changes label Nov 6, 2024
Mathlib/Data/Multiset/DershowitzManna.lean Outdated Show resolved Hide resolved
Mathlib/Data/Multiset/DershowitzManna.lean Outdated Show resolved Hide resolved
Mathlib/Data/Multiset/DershowitzManna.lean Outdated Show resolved Hide resolved
Mathlib/Data/Multiset/DershowitzManna.lean Outdated Show resolved Hide resolved
@YaelDillies YaelDillies added the awaiting-author A reviewer has asked the author a question or requested changes label Nov 6, 2024
@haitian-yuki haitian-yuki added awaiting-author A reviewer has asked the author a question or requested changes and removed awaiting-author A reviewer has asked the author a question or requested changes labels Nov 6, 2024
@haitian-yuki haitian-yuki removed the awaiting-author A reviewer has asked the author a question or requested changes label Nov 6, 2024
Comment on lines +25 to +26
- `Multiset.TransLT_eq_isDershowitzMannaLT` : two definitions of the `Dershowitz-Manna ordering`
are equivalent.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wouldn't mention this since it's private

Suggested change
- `Multiset.TransLT_eq_isDershowitzMannaLT` : two definitions of the `Dershowitz-Manna ordering`
are equivalent.

@YaelDillies
Copy link
Collaborator

I have just pushed a golf. I am running out of time today, but you should feel free to have a look for yourself at my commit and try golfing further using the tricks you will have learned. 😃

@haitian-yuki
Copy link
Collaborator Author

haitian-yuki commented Nov 15, 2024

I have just pushed a golf. I am running out of time today, but you should feel free to have a look for yourself at my commit and try golfing further using the tricks you will have learned. 😃

Hi Thanks for showing some of the golfing tricks;-) @YaelDillies . I tried golfing a little bit more and maybe now it is in a better form. One thing that I am still unsure about is whether I am allowed to use aesop in the proof as long as it doesn't show 'time run out' thing? Is that what you meant by 'so long as performance is not too terrible'? Thanks!

Ah I just saw some checks were not successful and I'm trying to understand what happened to the branch...

Copy link
Collaborator

@Rida-Hamadani Rida-Hamadani left a comment

Choose a reason for hiding this comment

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

To answer the question regarding aesop, you can write #count_heartbeats in 1 line before a lemma to see how much of a difference using aesop changes, relevant scale for heartbeats is 200000.

Mathlib/Data/Multiset/DershowitzManna.lean Outdated Show resolved Hide resolved
Mathlib/Data/Multiset/DershowitzManna.lean Outdated Show resolved Hide resolved
@YaelDillies
Copy link
Collaborator

Can you please merge master to get rid of the obscure CI error?

@haitian-yuki
Copy link
Collaborator Author

Thanks again for another golf @YaelDillies ! I have to say it looks pretty clean to me now and I cannot see much to improve here ;-) Does it mean the PR is done now? Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new-contributor This PR was made by a contributor with at most 5 merged PRs. Welcome to the community! t-data Data (lists, quotients, numbers, etc)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants