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] - refactor(data/*/interval): generalize finset.Ico to locally finite orders #7987

Closed
wants to merge 51 commits into from

Conversation

YaelDillies
Copy link
Collaborator

@YaelDillies YaelDillies commented Jun 18, 2021

finset.Ico currently goes ℕ → ℕ → finset ℕ. This generalizes it to α → α → finset α where locally_finite_order α.
This also ports all the existing finset.Ico lemmas to the new API, which involves renaming and moving them to data.finset.interval or data.nat.interval depending on whether I could generalize them away from or not.


Open in Gitpod

@YaelDillies YaelDillies added WIP Work in progress RFC Request for comment labels Jun 18, 2021
@pechersky
Copy link
Collaborator

I don't think you need multiset.Ixx because that's provided by finset.Ixx.val.

I don't understand why we would want to get rid of list.range', list.iota, and definitely not list.range and list.range_core. They're basic primitives for computation, used in tactics and testing. Would you propose that those files have to import this file just to be able to build a sequential list of nat?

The Ico function you have built is good for proving things about the properties of the function, but it is not good for performance. I think we should retain the list.range' functions as they are, and utilize them to provide the locally_finite_order instances for nat. Possibly also int with int.range. I just don't understand what the benefit is of getting rid of the list.range-like functions, so would be happy to hear your stance on it.

It might make sense to rename the class to just locally_finite since you're not providing anything about the order in this class, the order comes solely from the preorder or order_top.

@YaelDillies
Copy link
Collaborator Author

Noted for multiset.Ixx

My reasoning was that if we could achieve what those do (including in terms of performance), then we might as well use them. But I see we are nowhere near that. So, yeah, probably not worth doing.

There are many many things which could be qualified of locally_finite. I'm pretty sure we can allow a little more verbosity to disambiguate.

What's your opinion about whether the function field should be finset-valued or list-valued? We can always get the list from the finset using finset.sort, but that may not be the most efficient way to do it given that the way we prove the instances (at least for ℕ and ℤ) is actually to first find an enumerating list.

@b-mehta
Copy link
Collaborator

b-mehta commented Jun 18, 2021

There are many many things which could be qualified of locally_finite. I'm pretty sure we can allow a little more verbosity to disambiguate.

(for instance, mathlib already has locally finite topological spaces)

@YaelDillies YaelDillies added awaiting-review The author would like community review of the PR and removed RFC Request for comment WIP Work in progress labels Jun 20, 2021
@YaelDillies
Copy link
Collaborator Author

Okay so I think this is ready for a PR. I'm offering not to touch any other file yet, because there's already a good deal of material to be reviewed. If all goes well, I will move all the finset stuff in data.finset.intervals (unless you want me to do it now?).

@pechersky
Copy link
Collaborator

You're going to have to at least remove the definition of finset.Ico in data.finset.intervals, otherwise they collide (the reason the tests fail atm).

I don't think the API for set.intervals needs to be copied, because of the coe and mem lemmas, the inter-op shouldn't be too bad. If you're talking about unions, differences, intersections, then the results are reachable via the set API.

In subtype.locally_finite_order, I would make (p : Prop) explicit.

@YaelDillies
Copy link
Collaborator Author

Okay well then I have to also adapt the existing API in data.finset.intervals, so it's gonna take a while.

@YaelDillies YaelDillies added WIP Work in progress and removed awaiting-review The author would like community review of the PR labels Jun 22, 2021
@github-actions github-actions bot removed the merge-conflict Please `git merge origin/master` then a bot will remove this label. label Sep 13, 2021
@github-actions github-actions bot added the merge-conflict Please `git merge origin/master` then a bot will remove this label. label Sep 14, 2021
@YaelDillies YaelDillies mentioned this pull request Sep 14, 2021
1 task
@github-actions github-actions bot added the blocked-by-other-PR This PR depends on another PR which is still in the queue. A bot manages this label via PR comment. label Oct 4, 2021
@github-actions github-actions bot added the merge-conflict Please `git merge origin/master` then a bot will remove this label. label Oct 5, 2021
@github-actions github-actions bot removed merge-conflict Please `git merge origin/master` then a bot will remove this label. blocked-by-other-PR This PR depends on another PR which is still in the queue. A bot manages this label via PR comment. labels Oct 5, 2021
@YaelDillies YaelDillies 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 Oct 6, 2021
@github-actions github-actions bot added the merge-conflict Please `git merge origin/master` then a bot will remove this label. label Oct 6, 2021
@github-actions github-actions bot removed the merge-conflict Please `git merge origin/master` then a bot will remove this label. label Oct 6, 2021
@jcommelin
Copy link
Member

@YaelDillies This PR is touch 34 files. It is not exactly clear to me what it is doing. Could you please write a bit more extensive PR description?

@jcommelin jcommelin 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 Oct 8, 2021
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-author A reviewer has asked the author a question or requested changes labels Oct 8, 2021
bors bot pushed a commit that referenced this pull request Oct 8, 2021
…orders (#7987)

`finset.Ico` currently goes `ℕ → ℕ → finset ℕ`. This generalizes it to `α → α → finset α` where `locally_finite_order α`.
This also ports all the existing `finset.Ico` lemmas to the new API, which involves renaming and moving them to `data.finset.interval` or `data.nat.interval` depending on whether I could generalize them away from `ℕ` or not.



Co-authored-by: Yury G. Kudryashov <urkud@urkud.name>
@bors
Copy link

bors bot commented Oct 8, 2021

Pull request successfully merged into master.

Build succeeded:

@bors bors bot changed the title refactor(data/*/interval): generalize finset.Ico to locally finite orders [Merged by Bors] - refactor(data/*/interval): generalize finset.Ico to locally finite orders Oct 8, 2021
@bors bors bot closed this Oct 8, 2021
@bors bors bot deleted the localfinorder branch October 8, 2021 10:04
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

7 participants