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(set_theory/game): impartial games and the Sprague-Grundy theorem #3855

Closed
wants to merge 27 commits into from

Conversation

foxthomson
Copy link
Collaborator

@foxthomson foxthomson commented Aug 18, 2020

@foxthomson foxthomson added the awaiting-review The author would like community review of the PR label Aug 18, 2020
@rwbarton rwbarton changed the title feat(set_theory/game/position,set_theory/game/impartial,set_theory/game/nim): impartial games and the Sprague-Grundy theorem feat(set_theory/game): impartial games and the Sprague-Grundy theorem Aug 18, 2020
Co-authored-by: Aaron Anderson <65780815+awainverse@users.noreply.github.com>
@semorrison semorrison 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 Aug 19, 2020

/-- The definition of single-heap nim, which can be viewed as a pile of stones where each player can
take a positive number of stones from it on their turn. -/
noncomputable def nim : ordinal → pgame
Copy link
Collaborator

Choose a reason for hiding this comment

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

I get a warning definition 'nim' was incorrectly marked as noncomputable here. Do you?

Copy link
Collaborator

Choose a reason for hiding this comment

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

However removing it gives the error rec_fn_macro only allowed in meta definitions... Sounds like it may be a Lean bug?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I get this too, it should be noncomputable due to the quotient.out, right? Where would be best to flag this?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you try to minimise this as an example? Ideally we could even get a MWE that doesn't even import mathlib, but it would suffice to get a MWE that just depends on the current mathlib. Can you try just copy and pasting this definition into a file that just imports pgame, nth_rewrite and ordinal? Check also if the from blocks can by replaced by sorry without making the problem go away.

After that, create a new issue here on github, and make a post on zulip to bring it to people's attention.

Copy link
Collaborator

Choose a reason for hiding this comment

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

(Unfortunately I think we better get to the bottom of this before merging, as we don't want warning in mathlib.)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

@semorrison semorrison 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 Aug 20, 2020
@foxthomson foxthomson 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 Aug 20, 2020
@semorrison semorrison added blocked-by-other-PR This PR depends on another PR which is still in the queue. A bot manages this label via PR comment. 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 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 Aug 20, 2020
@foxthomson foxthomson 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 Aug 21, 2020
@semorrison
Copy link
Collaborator

Fantastic, thanks for this! 🎉

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 Aug 22, 2020
bors bot pushed a commit that referenced this pull request Aug 22, 2020
…#3855)

Co-authored-by: foxthomson <11833933+foxthomson@users.noreply.github.com>
@bors
Copy link

bors bot commented Aug 22, 2020

Pull request successfully merged into master.

Build succeeded:

@bors bors bot changed the title feat(set_theory/game): impartial games and the Sprague-Grundy theorem [Merged by Bors] - feat(set_theory/game): impartial games and the Sprague-Grundy theorem Aug 22, 2020
@bors bors bot closed this Aug 22, 2020
@bors bors bot deleted the impartial branch August 22, 2020 10:39
bors bot pushed a commit that referenced this pull request Aug 29, 2020
* Misc. style cleanups and code golf
* Changed naming and namespace to adhere more closely to the naming convention
* Changed `impartial` to be a `class`. I am aware that @semorrison explicitly requested not to make `impartial` a class in the #3855, but after working with the definition a bit I concluded that making it a class is worth it, simply because writing `impartial_add (nim_impartial _) (nim_impartial _)` gets annoying quite quickly, but also because you tend to get goal states of the form `Grundy_value _ = Grundy_value _`. By making `impartial` a class and making the game argument explicit, these goal states look like `grundy_value G = grundy_value H`, which is much nicer to work with.
robertylewis pushed a commit that referenced this pull request Aug 31, 2020
* Misc. style cleanups and code golf
* Changed naming and namespace to adhere more closely to the naming convention
* Changed `impartial` to be a `class`. I am aware that @semorrison explicitly requested not to make `impartial` a class in the #3855, but after working with the definition a bit I concluded that making it a class is worth it, simply because writing `impartial_add (nim_impartial _) (nim_impartial _)` gets annoying quite quickly, but also because you tend to get goal states of the form `Grundy_value _ = Grundy_value _`. By making `impartial` a class and making the game argument explicit, these goal states look like `grundy_value G = grundy_value H`, which is much nicer to work with.
PatrickMassot pushed a commit that referenced this pull request Sep 8, 2020
* Misc. style cleanups and code golf
* Changed naming and namespace to adhere more closely to the naming convention
* Changed `impartial` to be a `class`. I am aware that @semorrison explicitly requested not to make `impartial` a class in the #3855, but after working with the definition a bit I concluded that making it a class is worth it, simply because writing `impartial_add (nim_impartial _) (nim_impartial _)` gets annoying quite quickly, but also because you tend to get goal states of the form `Grundy_value _ = Grundy_value _`. By making `impartial` a class and making the game argument explicit, these goal states look like `grundy_value G = grundy_value H`, which is much nicer to work with.
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