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] - doc(order/game_add): improve docstrings #18269

Closed
wants to merge 2 commits into from

Conversation

vihdzp
Copy link
Collaborator

@vihdzp vihdzp commented Jan 23, 2023

We add simpler descriptions for game_add and well_founded.prod_game_add, without removing previous references to combinatorial games.

Mathlib 4 pair: leanprover-community/mathlib4#1798


Open in Gitpod

@vihdzp vihdzp added easy < 20s of review time. See the lifecycle page for guidelines. awaiting-review The author would like community review of the PR docs This PR is about documentation labels Jan 23, 2023
@vihdzp vihdzp added the modifies-synchronized-file This PR touches a files that has already been ported to mathlib4, and may need a synchronization PR. label Jan 23, 2023
Copy link
Collaborator

@alreadydone alreadydone left a comment

Choose a reason for hiding this comment

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

LGTM but have you opened a mathlib4 placeholder PR?

Comment on lines 39 to 40
It is so called, as it models game addition within combinatorial game theory. If `rα a₁ a₂` means
that `a₁ ⟶ a₂` is a valid move in game `α`, and `rβ b₁ b₂` means that `b₁ ⟶ b₂` is a valid move
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is counterintuitive, because in mathlib we use < rather than >. In terms of games, a₁ < a₂ means a₁ is simpler than a₂, and pgame.subsequent and pgame.is_option have argument orders consistent with this. So it would be better to keep the original order, saying rα a₁ a₂ means that a₂ ⟶ a₁ is a valid move. (You might restore the original notation a and a' if you want.)

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 didn't change the order, this was just a docstring typo. Also, will add the placeholder PR shortly.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry, what was the typo? And why do you say you didn't change it? pgame.is_option a' a means there's a valid move from a to a', not the other way around, right? So after your modification, the docstring makes wrong sense when applied to rα := pgame.is_option.

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 didn't redefine the function, just renamed variables in an attempt to match is_option and such, then accidentally swapped variables in the docstring.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, my comment is on the docstring from the start. Looks good now, thanks!

maintainer merge

@github-actions
Copy link

🚀 Pull request has been placed on the maintainer queue by alreadydone.

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

@leanprover-community-bot-assistant leanprover-community-bot-assistant 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 Jan 24, 2023
bors bot pushed a commit that referenced this pull request Jan 24, 2023
We add simpler descriptions for `game_add` and `well_founded.prod_game_add`, without removing previous references to combinatorial games.

Mathlib 4 pair: leanprover-community/mathlib4#1798
@bors
Copy link

bors bot commented Jan 24, 2023

Pull request successfully merged into master.

Build succeeded:

@bors bors bot changed the title doc(order/game_add): improve docstrings [Merged by Bors] - doc(order/game_add): improve docstrings Jan 24, 2023
@bors bors bot closed this Jan 24, 2023
@bors bors bot deleted the game_add_docs branch January 24, 2023 07:27
bors bot pushed a commit to leanprover-community/mathlib4 that referenced this pull request Jan 25, 2023
bors bot pushed a commit to leanprover-community/mathlib4 that referenced this pull request Jan 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs This PR is about documentation easy < 20s of review time. See the lifecycle page for guidelines. modifies-synchronized-file This PR touches a files that has already been ported to mathlib4, and may need a synchronization PR. 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