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/finset): provide the coercion finset α → Type* #7575

Closed
wants to merge 13 commits into from

Conversation

Vierkantor
Copy link
Collaborator

@Vierkantor Vierkantor commented May 11, 2021

There doesn't seem to be a good reason that finset doesn't have a coe_to_sort like set does. Before the change in this PR, we had to suffer the inconvenience of writing (↑s : set _) whenever we want the subtype of all elements of s. Now you can just write s.

I removed the obvious instances of the ((↑s : set _) : Type*) pattern, although it definitely remains in some places. I'd rather do those in separate PRs since it does not really do any harm except for being annoying to type. There are also some parts of the API (polynomial.root_set stands out to me) that could be designed around the use of finsets rather than sets that are later proved to be finite, which I again would like to declare out of scope.


Open in Gitpod

@Vierkantor Vierkantor added the WIP Work in progress label May 11, 2021
@riccardobrasca
Copy link
Member

Can you add finset.apply_coe_mem_map after finset.mem_map_of_mem as in #7566 ? I wanted to do it there but the coercion to type was missing.

@pechersky
Copy link
Collaborator

There's the obvious fintype instance which should be added on such a type.

@github-actions github-actions bot added the merge-conflict Please `git merge origin/master` then a bot will remove this label. label May 20, 2021
@Vierkantor
Copy link
Collaborator Author

It (finally!) builds on my machine now. Let's see what the linter thinks.

@github-actions github-actions bot removed the merge-conflict Please `git merge origin/master` then a bot will remove this label. label May 25, 2021
@Vierkantor Vierkantor added awaiting-review The author would like community review of the PR and removed WIP Work in progress labels May 25, 2021
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.

Looks good to me, but I think another pair of eyes on it would be good.

@semorrison
Copy link
Collaborator

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 May 30, 2021
@bors
Copy link

bors bot commented May 30, 2021

Merge conflict.

@github-actions github-actions bot added the merge-conflict Please `git merge origin/master` then a bot will remove this label. label May 30, 2021
@bryangingechen bryangingechen added awaiting-author A reviewer has asked the author a question or requested changes and removed merge-conflict Please `git merge origin/master` then a bot will remove this label. labels May 30, 2021
@github-actions github-actions bot added the merge-conflict Please `git merge origin/master` then a bot will remove this label. label May 30, 2021
@Vierkantor
Copy link
Collaborator Author

The conflict was just two additions in the same place, easy to fix. Currently recompiling to see if anything breaks.

This has been bugging me for a while and I have no good explanantion
why it was missing. I'll let the CI run for a bit to see if the linters
can explain why.

TODO: get rid of the (obvious?) instances of the old pattern
`(((s : finset α) : set α) : Type*)`
I definitely missed a number of them. I think I'll leave that for other cleanup
commits.
@github-actions github-actions bot removed the merge-conflict Please `git merge origin/master` then a bot will remove this label. label May 31, 2021
@bryangingechen bryangingechen added delegated The PR author may merge after reviewing final suggestions. and removed ready-to-merge All that is left is for bors to build and merge this PR. (Remember you need to say `bors r+`.) labels May 31, 2021
@bryangingechen
Copy link
Collaborator

Feel free to merge (again) if this builds!
bors d+

@bors
Copy link

bors bot commented May 31, 2021

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

@Vierkantor
Copy link
Collaborator Author

bors r+

@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 May 31, 2021
bors bot pushed a commit that referenced this pull request May 31, 2021
There doesn't seem to be a good reason that `finset` doesn't have a `coe_to_sort` like `set` does. Before the change in this PR, we had to suffer the inconvenience of writing `(↑s : set _)` whenever we want the subtype of all elements of `s`. Now you can just write `s`.

I removed the obvious instances of the `((↑s : set _) : Type*)` pattern, although it definitely remains in some places. I'd rather do those in separate PRs since it does not really do any harm except for being annoying to type. There are also some parts of the API (`polynomial.root_set` stands out to me) that could be designed around the use of `finset`s rather than `set`s that are later proved to be finite, which I again would like to declare out of scope.
@bors
Copy link

bors bot commented May 31, 2021

Build failed (retrying...):

bors bot pushed a commit that referenced this pull request May 31, 2021
There doesn't seem to be a good reason that `finset` doesn't have a `coe_to_sort` like `set` does. Before the change in this PR, we had to suffer the inconvenience of writing `(↑s : set _)` whenever we want the subtype of all elements of `s`. Now you can just write `s`.

I removed the obvious instances of the `((↑s : set _) : Type*)` pattern, although it definitely remains in some places. I'd rather do those in separate PRs since it does not really do any harm except for being annoying to type. There are also some parts of the API (`polynomial.root_set` stands out to me) that could be designed around the use of `finset`s rather than `set`s that are later proved to be finite, which I again would like to declare out of scope.
@bors
Copy link

bors bot commented May 31, 2021

Build failed (retrying...):

bors bot pushed a commit that referenced this pull request May 31, 2021
There doesn't seem to be a good reason that `finset` doesn't have a `coe_to_sort` like `set` does. Before the change in this PR, we had to suffer the inconvenience of writing `(↑s : set _)` whenever we want the subtype of all elements of `s`. Now you can just write `s`.

I removed the obvious instances of the `((↑s : set _) : Type*)` pattern, although it definitely remains in some places. I'd rather do those in separate PRs since it does not really do any harm except for being annoying to type. There are also some parts of the API (`polynomial.root_set` stands out to me) that could be designed around the use of `finset`s rather than `set`s that are later proved to be finite, which I again would like to declare out of scope.
@bors
Copy link

bors bot commented Jun 1, 2021

Pull request successfully merged into master.

Build succeeded:

@bors bors bot changed the title feat(data/finset): provide the coercion finset α → Type* [Merged by Bors] - feat(data/finset): provide the coercion finset α → Type* Jun 1, 2021
@bors bors bot closed this Jun 1, 2021
@bors bors bot deleted the coe_to_sort-finset branch June 1, 2021 01:52
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. 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