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(slim_check): sampleable instance for generating functions and injective functions #3967

Closed
wants to merge 36 commits into from

Conversation

cipher1024
Copy link
Collaborator

@cipher1024 cipher1024 commented Aug 28, 2020

This also adds printable_prop to trace why propositions hold or don't hold and sampleable_ext to allow the data structure generated and shrunken by slim_check to have a different type from the type we are looking for.


@cipher1024 cipher1024 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 Aug 28, 2020
@github-actions github-actions bot added the merge-conflict Please `git merge origin/master` then a bot will remove this label. label Sep 1, 2020
@bryangingechen bryangingechen changed the title feat(slim_check): new subtype instances of sampleable (blocked by #3915) feat(slim_check): new subtype instances of sampleable Sep 2, 2020
@bryangingechen bryangingechen removed 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 Sep 2, 2020
@github-actions github-actions bot removed the merge-conflict Please `git merge origin/master` then a bot will remove this label. label Sep 9, 2020
@github-actions github-actions bot added the merge-conflict Please `git merge origin/master` then a bot will remove this label. label Sep 10, 2020
@github-actions github-actions bot removed the merge-conflict Please `git merge origin/master` then a bot will remove this label. label Sep 11, 2020
@semorrison semorrison added the awaiting-review The author would like community review of the PR label Sep 11, 2020
src/data/list/zip.lean Outdated Show resolved Hide resolved
test/slim_check.lean Outdated Show resolved Hide resolved
Co-authored-by: Bryan Gin-ge Chen <bryangingechen@gmail.com>
Copy link
Member

@robertylewis robertylewis left a comment

Choose a reason for hiding this comment

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

The tests look really nice, there are some interesting features added here. But I'll admit that again I find this PR very difficult to review. I don't have a sense of what al the high-level additions are.

From the new tests, it's clear there are new testable instances that let slim_check work on Prop goals and function properties, which is great, and I see instances that add this (and a module doc specifically about functions). Are these features independent or related? I also see instances about functors -- is that part of these features? Another feature that doesn't appear in the tests? Are there other added features?

Basically, this is 1300 lines where my only guidance for reviewing is "new subtype instances of sampleable" and some tests. Besides my minor comments the code looks stylistically fine. And again the tests look great, having slim_check work on prop logic goals is really nice. But I don't know how to say much about the code as a whole because I can't separate the changes from the new features or the new features from each other.

Are all the changes here too intertwined to separate them? Ideally, there would be a PR that just adds the prop logic functionality (and tests), one that adds the function instances (and tests), etc.

src/tactic/slim_check.lean Show resolved Hide resolved
src/testing/slim_check/functions.lean Show resolved Hide resolved
src/testing/slim_check/functions.lean Outdated Show resolved Hide resolved
src/testing/slim_check/functions.lean Show resolved Hide resolved
src/testing/slim_check/sampleable.lean Outdated Show resolved Hide resolved
src/testing/slim_check/sampleable.lean Outdated Show resolved Hide resolved
@robertylewis robertylewis 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 Sep 25, 2020
@cipher1024
Copy link
Collaborator Author

The bulk of this PR is to allow me to generate samples of pi types. Because pi types mostly cannot have has_to_string and has_repr instances and cannot be deconstructed, I created the sampleable_ext class where every instance also has a proxy type. For a function, the proxy type is a table that will hold the entries of the function. That table can be printed and shrunk and makes pi types sampleable. The sampleable class remains and is a simpler class to instantiate if you don't need the whole proxy type business.

sampleable_functor is a simple way to define sampleable instances for collections like list. If you have a list of functions, the sampleable_ext instance will be chosen for that. Otherwise, it will go with sampleable instances.

@cipher1024 cipher1024 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 Sep 25, 2020
Copy link
Member

@robertylewis robertylewis left a comment

Choose a reason for hiding this comment

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

@cipher1024 I'm sorry, I just don't know how to review this properly. There are too many changes in too many different places, and I don't have a good sense of what single goal the PR is trying to accomplish.

Do you promise that every new feature is documented and has multiple tests? If so: if you can address my couple comments here and change the PR title to show what features have been added, you can merge. But please, really try to split these PRs up as much as you can. If code needs to be refactored for a new feature to work, you can do the refactoring in one PR and the new feature in another. If two features are related but not the same, they should be two different PRs. I can't look at a diff this size and figure out myself the reasons for the changes.

bors d+

src/testing/slim_check/functions.lean Show resolved Hide resolved
src/testing/slim_check/sampleable.lean Show resolved Hide resolved
@bors
Copy link

bors bot commented Sep 28, 2020

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

@robertylewis robertylewis 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 Sep 28, 2020
@cipher1024 cipher1024 changed the title feat(slim_check): new subtype instances of sampleable feat(slim_check): sampleable instance for generating functions and injective functions Oct 3, 2020
@cipher1024
Copy link
Collaborator Author

@robertylewis I promise all I did was tested and motivated by the tests I wrote. I take your point however and will try to break things up a bit more. Now that the basics for slim_check are in mathlib, I think it should get easier

@cipher1024
Copy link
Collaborator Author

And without further ado, I'm now going to merge it. Thanks @robertylewis !

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 Oct 3, 2020
bors bot pushed a commit that referenced this pull request Oct 3, 2020
…jective functions (#3967)

This also adds `printable_prop` to trace why propositions hold or don't hold and `sampleable_ext` to allow the data structure generated and shrunken by `slim_check` to have a different type from the type we are looking for.
@bors
Copy link

bors bot commented Oct 3, 2020

Build failed (retrying...):

bors bot pushed a commit that referenced this pull request Oct 3, 2020
…jective functions (#3967)

This also adds `printable_prop` to trace why propositions hold or don't hold and `sampleable_ext` to allow the data structure generated and shrunken by `slim_check` to have a different type from the type we are looking for.
@bors
Copy link

bors bot commented Oct 3, 2020

Pull request successfully merged into master.

Build succeeded:

@bors bors bot changed the title feat(slim_check): sampleable instance for generating functions and injective functions [Merged by Bors] - feat(slim_check): sampleable instance for generating functions and injective functions Oct 3, 2020
@bors bors bot closed this Oct 3, 2020
@bors bors bot deleted the slim-check-instances branch October 3, 2020 20:51
adomani pushed a commit that referenced this pull request Oct 7, 2020
…jective functions (#3967)

This also adds `printable_prop` to trace why propositions hold or don't hold and `sampleable_ext` to allow the data structure generated and shrunken by `slim_check` to have a different type from the type we are looking for.
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

4 participants