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

[Feature,Refactor] Chess improvements: fen, pgn, pixels, san #2702

Merged
merged 10 commits into from
Jan 26, 2025

Conversation

[ghstack-poisoned]
Copy link

pytorch-bot bot commented Jan 17, 2025

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/rl/2702

Note: Links to docs will display an error until the docs builds have been completed.

This comment was automatically generated by Dr. CI and updates every 15 minutes.

vmoens added a commit that referenced this pull request Jan 17, 2025
ghstack-source-id: 81dba82c14ae94c3c25b1a2e78fc3b478cddcc8d
Pull Request resolved: #2702
@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jan 17, 2025
@vmoens vmoens requested a review from kurtamohler January 17, 2025 17:01
@vmoens vmoens added enhancement New feature or request Refactoring Refactoring of an existing feature labels Jan 17, 2025
[ghstack-poisoned]
vmoens added a commit that referenced this pull request Jan 17, 2025
ghstack-source-id: 87c458ebe69a21569719d12ad17fb3d7a356da0d
Pull Request resolved: #2702
# Generate the PGN string
pgn_string = str(game)
return pgn_string

@classmethod
def _get_fen(cls, tensordict):
Copy link
Collaborator

@kurtamohler kurtamohler Jan 17, 2025

Choose a reason for hiding this comment

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

Maybe this function can be removed, in favor of a direct TensorDict.get call?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking that one day we might let people choose the fen / pgn key for themselves but it's true that as of now it's not incredibly useful

fen = self._get_fen(tensordict).data
dest = tensordict.empty()
if self.include_pgn:
fen = self._get_pgn(tensordict).data
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
fen = self._get_pgn(tensordict).data
pgn = self._get_pgn(tensordict).data

Also _get_pgn doesn't exist, but I figure you realize that

@classmethod
def _get_tensor_image(cls, board):
try:
svg = board._repr_svg_()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we could have another option that has a more minimal representation as well, if we want to train without vision. For instance, OpenSpiel's chess env has a representation where each square of the board is given a size 20 array of ones and zeros, which is somehow used to represent which piece is in that square and which color it is

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I was using this because it's builtin - but a more minimal representation could also be cool!

[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
@vmoens vmoens merged commit 2f4b302 into gh/vmoens/71/base Jan 26, 2025
2 checks passed
vmoens added a commit that referenced this pull request Jan 26, 2025
…mask

ghstack-source-id: f294a2bc99a17911c9b62558d530b148d3c0350f
Pull Request resolved: #2702
@vmoens vmoens deleted the gh/vmoens/71/head branch January 26, 2025 23:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. enhancement New feature or request Refactoring Refactoring of an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants