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 game german_whist_foregame #1171

Merged
merged 31 commits into from
Oct 10, 2024

Conversation

willmcgowan
Copy link
Contributor

German Whist is a 2 player imperfect information trick-taking card game. The rules are outlined here https://en.wikipedia.org/wiki/German_whist#:~:text=German%20whist%20is%20a%20variant,is%20probably%20of%20British%20origin.

Modified from previous pull request, no longer requires x86intrin.h or bmi2 support.

@willmcgowan
Copy link
Contributor Author

I hadn't added the game name to the pyspiel tests expected games.

Removed MTD as it slows generation of tablebase.
Removed move ordering heuristics from legalactions as it slows tablebase generation.
Hacky move ordering heuristics remain for speeding up verification.
Tablebase generation is 50% faster
Modified bzhi bithack so it will compile to bzhi when __bmi2__ is defined.
Modified pext bithack so it will call _pext_u64 when __bmi2__ is defined, otherwise it will use the bithack.
LoadTTable now warns on failing to load and sets TTable to default value(all 0)
@willmcgowan
Copy link
Contributor Author

@lanctot Should I remove ismcts_gwhist from the PR?

@lanctot
Copy link
Collaborator

lanctot commented Feb 6, 2024

@lanctot Should I remove ismcts_gwhist from the PR?

No it's ok, you can leave it. Good to have an example and it's in the right place.

@lanctot
Copy link
Collaborator

lanctot commented Feb 13, 2024

I don't know what is going on but the tests keep failing repeatedly after your last commit. Can you pull changes from master and push the merge commit? (maybe make an inconsequential change to one of your files if necessary to trigger the tests)

Apologies on the lateness in looking at this, I will take a look as soon as the tests pass.


inline const std::array<uint64_t, 4> kSuitMasks = { bzhi_u64(~0,kNumRanks),bzhi_u64(~0,2 * kNumRanks) ^ bzhi_u64(~0,kNumRanks),bzhi_u64(~0,3 * kNumRanks) ^ bzhi_u64(~0,2 * kNumRanks),bzhi_u64(~0,4 * kNumRanks) ^ bzhi_u64(~0,3 * kNumRanks) };


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 add some high-level comments to these structures and methods, just generally to explain what they are used for and what they do?

void SetChar(size_t i,size_t j,char value);
char Get(size_t i,size_t j) const;
void Set(size_t i,size_t j,char value);
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe take a quick look at Google C++ style. Each line should be at most 80 characters. There are automated linters you can use (see the docs for adding a game, step 9)

Also should be blank lines between definitions of classes and functions (see the other game implementations for examples)

@willmcgowan
Copy link
Contributor Author

My apologies for the delay in getting back to you, I have issues getting the python tests to run on my device due to some issues with absl.
api_test.py and the playthrough_test.py are failing. With regard to api_test.py I have added an assert in the informationstatestring implementation.
I can add some more extensive comments and linting by monday.

Added playthrough with incorrect tablebase value.
Checks for InfoStateString and ObservationString
@willmcgowan
Copy link
Contributor Author

Sorry for taking so long to address some of the issues.
On my device the playthrough test is failing throwing this error:

**Traceback (most recent call last):
File "/Users/thomasmcgowan/Documents/Github/open_spiel/open_spiel/python/../integration_tests/playthrough_test.py", line 90, in test_all_games_tested
missing_games = set(_AVAILABLE_GAMES) - set(_MISSING_GAMES) - set(
File "/Users/thomasmcgowan/Documents/Github/open_spiel/open_spiel/python/../integration_tests/playthrough_test.py", line 91, in
_playthrough_match(os.path.join(path, basename), _SHORTNAME)[1]
File "/Users/thomasmcgowan/Documents/Github/open_spiel/open_spiel/python/../integration_tests/playthrough_test.py", line 57, in _playthrough_match
data = f.read()
File "/Library/Developer/CommandLineTools/Library/Frameworks/Python3.framework/Versions/3.9/lib/python3.9/codecs.py", line 322, in decode
(result, consumed) = self._buffer_decode(data, self.errors, final)
UnicodeDecodeError: 'utf-8' codec can't decode byte 0x80 in position 3131: invalid start byte

======================================================================
ERROR: test_playthrough_.DS_Store (main.PlaythroughTest)
PlaythroughTest.test_playthrough_.DS_Store

Traceback (most recent call last):
File "/Users/thomasmcgowan/Documents/Github/open_spiel/open_spiel/python/../integration_tests/playthrough_test.py", line 108, in
test_func = lambda self, basename=basename: self.run_test(path, basename)
File "/Users/thomasmcgowan/Documents/Github/open_spiel/open_spiel/python/../integration_tests/playthrough_test.py", line 76, in run_test
expected, actual = generate_playthrough.replay(file_path)
File "/Users/thomasmcgowan/Documents/Github/open_spiel/open_spiel/python/algorithms/generate_playthrough.py", line 526, in replay
original, kwargs = _read_playthrough(filename)
File "/Users/thomasmcgowan/Documents/Github/open_spiel/open_spiel/python/algorithms/generate_playthrough.py", line 519, in _read_playthrough
original = f.read()
File "/Library/Developer/CommandLineTools/Library/Frameworks/Python3.framework/Versions/3.9/lib/python3.9/codecs.py", line 322, in decode
(result, consumed) = self._buffer_decode(data, self.errors, final)
UnicodeDecodeError: 'utf-8' codec can't decode byte 0x80 in position 3131: invalid start byte

Investigating with hexdump:
xxd -l 16 -s 3131 ./open_spiel/integration_tests/playthroughs/german_whist_foregame.txt
00000c3b: 3139 3630 3738 292c 2028 3134 2c30 2e30 196078), (14,0.0
This is the segment (where the chance outcomes are being printed).
I thought perhaps because my infostate string/observation state string made use of "/n" that might be causing the issue.

@lanctot lanctot added ready to import Ready to import imported This PR has been imported and awaiting internal review. Please avoid any more local changes, thanks! and removed imported This PR has been imported and awaiting internal review. Please avoid any more local changes, thanks! ready to import Ready to import labels Apr 16, 2024
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.

Hi @willmcgowan, sorry but I missed a few things that are critical. I had started the import but the changes required are non-trivial. Apologies for the delay.

@lanctot lanctot added imported This PR has been imported and awaiting internal review. Please avoid any more local changes, thanks! and removed imported This PR has been imported and awaiting internal review. Please avoid any more local changes, thanks! labels Oct 9, 2024
@lanctot
Copy link
Collaborator

lanctot commented Oct 10, 2024

Apologies for the delays.. the summer ended up a bit busier than I expected!

Should finally be merged some time soon (today or next few days).

@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 Oct 10, 2024
@lanctot lanctot closed this Oct 10, 2024
@lanctot lanctot reopened this Oct 10, 2024
@lanctot lanctot merged commit a66063d into google-deepmind:master Oct 10, 2024
18 checks passed
@lanctot
Copy link
Collaborator

lanctot commented Oct 17, 2024

Hi @willmcgowan, we had to remove the game because it was crashing in the playthrough test.

I tried a few things but ultimately could not fix it. I believe it might be due to the game construction trying to read the transposition table from disk or maybe taking up too much memory constructing the initial TTable.

Here's the commit: a66063d

We should have a version of the game that will work without a transposition table, i.e. make it optional by default.

If you want to take a look please start from the fixed version in this commit above. I had to make a number of changes for it to fit our internal style and the version above includes all of them. If you find and fix it, then please resubmit as a new PR.

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