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

adding domino #1018

Merged
merged 21 commits into from
Mar 27, 2023
Merged

adding domino #1018

merged 21 commits into from
Mar 27, 2023

Conversation

morLev
Copy link
Contributor

@morLev morLev commented Feb 21, 2023

No description provided.

lanctot and others added 4 commits February 21, 2023 16:59
PiperOrigin-RevId: 511474154
Change-Id: I8a8272d6c7cc819f6cfb79c651e0d2058186af99
PiperOrigin-RevId: 511475596
Change-Id: I6808a939cdd21acb9b2e9b912575118afd249349
@lanctot
Copy link
Collaborator

lanctot commented Feb 22, 2023

Thanks!

#1017 has been merged into master, so the problems with the tests are now fixed. Can you pull changes from master and push the merge commit?

@morLev
Copy link
Contributor Author

morLev commented Feb 23, 2023

Thanks!

#1017 has been merged into master, so the problems with the tests are now fixed. Can you pull changes from master and push the merge commit?

Done

@lanctot
Copy link
Collaborator

lanctot commented Feb 23, 2023

Done

Hmm, oddly .. the new commits are showing as part of your PR now. Ok, we'll try this and see what happens, and hopefully they get marked as reverted during the import.

I started the tests now. If they pass, then someone will do a review soon. Thanks again!

@lanctot
Copy link
Collaborator

lanctot commented Feb 23, 2023

Hi @morLev, the tests failed because there is no playthrough for your new game.

Can you take a look at step 9 from these instructions: https://github.com/deepmind/open_spiel/blob/master/docs/developer_guide.md#adding-a-game to generate a new playthrough, then add it to your PR?

@morLev
Copy link
Contributor Author

morLev commented Feb 27, 2023

Hi @morLev, the tests failed because there is no playthrough for your new game.

Can you take a look at step 9 from these instructions: https://github.com/deepmind/open_spiel/blob/master/docs/developer_guide.md#adding-a-game to generate a new playthrough, then add it to your PR?

done

@lanctot
Copy link
Collaborator

lanctot commented Feb 28, 2023

The domino_test is now failing when run on Github Actions. You can see the output by clicking on the failure link above. Does it work for you locally?

@morLev
Copy link
Contributor Author

morLev commented Mar 1, 2023

The domino_test is now failing when run on Github Actions. You can see the output by clicking on the failure link above. Does it work for you locally?

Hi,

Sorry, it was not working locally.
solve it by modifying returns() (to return [0, 0] on nonterminal states).

@lanctot
Copy link
Collaborator

lanctot commented Mar 1, 2023

Great, thanks. Tests are passing now. Someone will do a review soon. Once any changes are made from review, we can import it.

Copy link
Collaborator

@lanctot lanctot left a comment

Choose a reason for hiding this comment

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

A few quick comments now.

open_spiel/python/games/domino.py Outdated Show resolved Hide resolved
open_spiel/python/games/domino.py Outdated Show resolved Hide resolved
open_spiel/python/games/domino.py Outdated Show resolved Hide resolved
open_spiel/python/games/domino.py Outdated Show resolved Hide resolved
open_spiel/python/games/domino_test.py Outdated Show resolved Hide resolved
open_spiel/python/games/domino.py Outdated Show resolved Hide resolved
open_spiel/python/games/domino.py Outdated Show resolved Hide resolved
open_spiel/python/games/domino.py Outdated Show resolved Hide resolved
open_spiel/python/games/domino_test.py Outdated Show resolved Hide resolved
open_spiel/python/games/domino.py Outdated Show resolved Hide resolved
open_spiel/python/games/domino.py Outdated Show resolved Hide resolved
open_spiel/python/games/domino.py Outdated Show resolved Hide resolved
@lanctot
Copy link
Collaborator

lanctot commented Mar 21, 2023

Thanks for the updates @morLev ! Can you respond to the threads now in the PR and mark any as resolved where appropriate?

@morLev
Copy link
Contributor Author

morLev commented Mar 22, 2023

@lanctot
@jhtschultz

Hi,

Besides resolving your comments, I also change how the first player to play is chosen (or more accurately, what hand is playing first).
did it because the rules I know are not documented anywhere online.
so now player 0 is always playing first, which is like a coin toss or drawing lots:

Drawing Lots to Determine Who Will Make the First Play:
After the tiles are shuffled, each player draws a domino from the stock. The player who draws the heaviest tile will make the first play. If there is a tie, it is broken by drawing new dominoes from the stock.
(https://www.dominorules.com/the-basics)

@lanctot
Copy link
Collaborator

lanctot commented Mar 22, 2023

Thanks @morLev !

open_spiel/python/games/domino.py Outdated Show resolved Hide resolved
open_spiel/python/CMakeLists.txt Outdated Show resolved Hide resolved
open_spiel/python/games/domino.py Outdated Show resolved Hide resolved
open_spiel/python/games/domino.py Outdated Show resolved Hide resolved
open_spiel/python/games/domino.py Outdated Show resolved Hide resolved
open_spiel/python/games/domino.py Outdated Show resolved Hide resolved
open_spiel/python/games/domino.py Outdated Show resolved Hide resolved
open_spiel/python/games/domino.py Outdated Show resolved Hide resolved
open_spiel/python/games/domino_test.py Outdated Show resolved Hide resolved
open_spiel/python/games/domino_test.py Outdated Show resolved Hide resolved
@lanctot lanctot added the imported This PR has been imported and awaiting internal review. Please avoid any more local changes, thanks! label Mar 24, 2023
@jhtschultz
Copy link
Member

Awesome looks good! Thanks @morLev! Great to have a dominoes game in Open Spiel :)

@lanctot lanctot added the merged internally The code is now submitted to our internal repo and will be merged in the next github sync. label Mar 24, 2023
@lanctot lanctot merged commit c8eaa57 into google-deepmind:master Mar 27, 2023
@lanctot lanctot mentioned this pull request Jun 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
imported This PR has been imported and awaiting internal review. Please avoid any more local changes, thanks! merged internally The code is now submitted to our internal repo and will be merged in the next github sync.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants