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] - chore(data/multiset/nodup): remove dependency on multiset.powerset #17888

Closed
wants to merge 6 commits into from

Conversation

FR-vdash-bot
Copy link
Collaborator

@FR-vdash-bot FR-vdash-bot commented Dec 10, 2022

This flips the import direction between data.multiset.powerset and data.multiset.nodup, such that the former now imports the latter, moving lemmas as necessary.

This matches how data.list.sublists (transitively) imports data.list.nodup.

More importantly, this means that finset (which needs multiset.nodup) can be defined without needing the material on list.sublists and multiset.powerset.


Open in Gitpod

@FR-vdash-bot FR-vdash-bot added awaiting-review The author would like community review of the PR awaiting-CI The author would like to see what CI has to say before doing more work. labels Dec 10, 2022
@eric-wieser
Copy link
Member

eric-wieser commented Dec 10, 2022

What's the motivation for this move? It's not clear to me that either of nodup or powerset is the more primitive operation, which favors doing nothing to avoid churn.

Of course, if you wrote a PR description explaining your motivation you might be able to persuade me otherwise :)

@FR-vdash-bot
Copy link
Collaborator Author

data.multiset.powerset imports more things (data.list.sublists) than data.multiset.nodup. data.multiset.nodup is not avoidable for finset, we want to make it import less things.

@eric-wieser
Copy link
Member

Ah thanks; finset needs nodup but not powerset.

I think this change makes sense; can you fix the build failure?

@eric-wieser
Copy link
Member

eric-wieser commented Dec 10, 2022

I've copy-edited your PR description to emphasize the important bits.

Can you add a assert_not_exists list.sublists to finset/basic.lean, assuming it doesn't error?

@eric-wieser eric-wieser changed the title chore(data/multiset/nodup): reduce imports chore(data/multiset/nodup): remove dependency on multiset.powerset Dec 10, 2022
@eric-wieser
Copy link
Member

bors d+

Thanks!

@bors
Copy link

bors bot commented Dec 10, 2022

✌️ negiizhao 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 Dec 10, 2022
@FR-vdash-bot
Copy link
Collaborator Author

list.sublists is in data.list.defs. I added assert_not_exists list.sublists_len.

@eric-wieser
Copy link
Member

Can you add a comment to that effect above the assert_not_exists?

Co-authored-by: Eric Wieser <wieser.eric@gmail.com>
@github-actions github-actions bot removed the awaiting-CI The author would like to see what CI has to say before doing more work. label Dec 10, 2022
@FR-vdash-bot
Copy link
Collaborator Author

bors r+

bors bot pushed a commit that referenced this pull request Dec 10, 2022
…17888)

This flips the import direction between `data.multiset.powerset` and `data.multiset.nodup`, such that the former now imports the latter, moving lemmas as necessary.

This matches how `data.list.sublists` (transitively) imports `data.list.nodup`.

More importantly, this means that `finset` (which needs `multiset.nodup`) can be defined without needing the material on `list.sublists` and `multiset.powerset`.
@bors
Copy link

bors bot commented Dec 10, 2022

Pull request successfully merged into master.

Build succeeded:

@bors bors bot changed the title chore(data/multiset/nodup): remove dependency on multiset.powerset [Merged by Bors] - chore(data/multiset/nodup): remove dependency on multiset.powerset Dec 10, 2022
@bors bors bot closed this Dec 10, 2022
@bors bors bot deleted the FR_multiset_imports branch December 10, 2022 22:50
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants