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

Add game: EinStein würfelt nicht! #1289

Merged
merged 9 commits into from
Dec 16, 2024

Conversation

giogix2
Copy link
Contributor

@giogix2 giogix2 commented Nov 17, 2024

Here's my implementation of the game EinStein würfelt nicht!

I've implemented only the basic version of the game with a 5x5 board, as described in the wiki https://en.wikipedia.org/wiki/EinStein_w%C3%BCrfelt_nicht!. I'm not familiar with the other versions, so for now I decided to stick with the simplest version.

There is a common variant where a player wins the game after winning a total of 3 games. In this implementation, winning a single match means winning the game.

Feedbacks are appreciated. Hopefully this game is a fit for open_spiel.

@lanctot
Copy link
Collaborator

lanctot commented Nov 21, 2024

Nice!! Thanks!

@lanctot
Copy link
Collaborator

lanctot commented Nov 22, 2024

Error looks unrelated to your changes and broken in some of our code. We'll look to get this fixed and let you know when master is up to date with fixes!

@giogix2
Copy link
Contributor Author

giogix2 commented Nov 22, 2024

Thank you for the heads up. I was planning to dive deeper into this in the next few days, but I was also confused, because tests pass for macos-14, but not for the others.
I'll wait for the changes in the master branch.

@lanctot
Copy link
Collaborator

lanctot commented Nov 22, 2024

Thank you for the heads up. I was planning to dive deeper into this in the next few days, but I was also confused, because tests pass for macos-14, but not for the others. I'll wait for the changes in the master branch.

Ok fixes applied. Can you pull the most recent changes from master and push the merge commit?

@giogix2
Copy link
Contributor Author

giogix2 commented Nov 23, 2024

Thank you for the heads up. I was planning to dive deeper into this in the next few days, but I was also confused, because tests pass for macos-14, but not for the others. I'll wait for the changes in the master branch.

Ok fixes applied. Can you pull the most recent changes from master and push the merge commit?

Done.

@giogix2
Copy link
Contributor Author

giogix2 commented Nov 24, 2024

Some jobs are still failing. Could this still be problem unrelated to these changes? It's a bit difficult to understand what is the root cause from the logs.

Though, there is one test failing for which the reason is clear, specifically: build_and_test / build (ubuntu-22.04, 3.10, OFF, OFF, OFF). This test fails because the initial board setup differs from the one in the playthrough file. In this game implementation, the position of the cubes on the board is initialized randomly, controlled by a random seed passed as an argument. If the seed is set to -1, it’s taken from the clock; otherwise, the default value is 42. This is done before the first move, at the constructor level.
When running a playthrough with the default parameters, I’d expect the initial setup to be the same (the playthrough tests always pass locally, note: I am using macOS and Python 3.10.15). However, the playthrough tests fail on Ubuntu 22.04 with Python 3.10. Could the random generator produce different values on different operating systems?
And maybe all the tests are failing because of this even if not specifically mentioned in the logs...

If this is the case, I might need to modify the implementation so that the board starts empty, and the first random action is to set up the board. This concept seems similar to what you're doing in Backgammon here.

@giogix2
Copy link
Contributor Author

giogix2 commented Nov 30, 2024

I modified the logic for the initial board setup. Previously, the board was initialized randomly using std::shuffle() within the constructor. Now, the allocation of cubes is determined by the values from the chance outcomes, removing the random behavior based on the environment.
I retested the playthrough on both macOS and Ubuntu, and it passes in both cases.

@lanctot could you please let the CI/CD run again to see if it passes this time?

@lanctot
Copy link
Collaborator

lanctot commented Nov 30, 2024

I modified the logic for the initial board setup. Previously, the board was initialized randomly using std::shuffle() within the constructor. Now, the allocation of cubes is determined by the values from the chance outcomes, removing the random behavior based on the environment. I retested the playthrough on both macOS and Ubuntu, and it passes in both cases.

@lanctot could you please let the CI/CD run again to see if it passes this time?

Sorry I had missed your last comment. This is indeed the right way to do the random starting board (via chance nodes).

@giogix2
Copy link
Contributor Author

giogix2 commented Nov 30, 2024

I modified the logic for the initial board setup. Previously, the board was initialized randomly using std::shuffle() within the constructor. Now, the allocation of cubes is determined by the values from the chance outcomes, removing the random behavior based on the environment. I retested the playthrough on both macOS and Ubuntu, and it passes in both cases.
@lanctot could you please let the CI/CD run again to see if it passes this time?

Sorry I had missed your last comment. This is indeed the right way to do the random starting board (via chance nodes).

Yes. I didn't realised this initially. But the integration issue above forced me to look at implementation of other similar games and I understood what was wrong.

@lanctot
Copy link
Collaborator

lanctot commented Dec 1, 2024

Something weird is happening with Github Actions CI. The output is odd... Python 3.11 is listing as available via brew but then won't install.

Not sure what's going on. I'll try it again in a few days.

(Could be that something changed in the CI images, but likely just a blip)

@lanctot
Copy link
Collaborator

lanctot commented Dec 1, 2024

Something weird is happening with Github Actions CI. The output is odd... Python 3.11 is listing as available via brew but then won't install.

Not sure what's going on. I'll try it again in a few days.

(Could be that something changed in the CI images, but likely just a blip)

Ahh, I know why... one of my hacks stopped working on github actions CI. That happens 😅

I'm a bit busy at the moment but I'll fix it mid week and update you when it's done. Apologies.

@giogix2
Copy link
Contributor Author

giogix2 commented Dec 1, 2024

Something weird is happening with Github Actions CI. The output is odd... Python 3.11 is listing as available via brew but then won't install.
Not sure what's going on. I'll try it again in a few days.
(Could be that something changed in the CI images, but likely just a blip)

Ahh, I know why... one of my hacks stopped working on github actions CI. That happens 😅

I'm a bit busy at the moment but I'll fix it mid week and update you when it's done. Apologies.

No problem at all. Thank you for looking into it.

@lanctot
Copy link
Collaborator

lanctot commented Dec 5, 2024

I'm a bit busy at the moment but I'll fix it mid week and update you when it's done. Apologies.

Ok, the fix is now in the master branch. Can you pull changes from master and push the merge commit to trigger the CI tests again?

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.

The code looks very good, thanks!

I have only one major requested change about how the chance node works.

@lanctot lanctot added 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. labels Dec 16, 2024
@lanctot lanctot merged commit 4907fa2 into google-deepmind:master Dec 16, 2024
10 checks passed
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.

2 participants