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 Spades #1233

Merged
merged 2 commits into from
May 31, 2024
Merged

Adding Spades #1233

merged 2 commits into from
May 31, 2024

Conversation

i-Madsen
Copy link
Contributor

Spades implementation is largely based off of the Bridge implementation. Runs a single round and returns the points earned/lost from the round (not the overall team's score). Note that currently the parameters use_mercy_rule, mercy_threshold, and win_threshold are not actually used at the moment. They will most likely be moved to and managed by the training script. However, score_partnership_0 and score_partnership_1 are needed in order to determine if a bag penalty is earned from the round.

Spades implementation is largely based off of the Bridge implementation. Runs a single round and returns the points earned/lost from the round (not the overall team's score). Note that currently the parameters use_mercy_rule, mercy_threshold, and win_threshold are not actually used at the moment. They will most likely be moved to and managed by the training script. However, score_partnership_0 and score_partnership_1 are needed in order to determine if a bag penalty is earned from the round.
@lanctot
Copy link
Collaborator

lanctot commented May 24, 2024

Can you generate a new playthrough and add it to the PR?

https://github.com/google-deepmind/open_spiel/blob/master/open_spiel/scripts/generate_new_playthrough.sh

(Tests are failing without it)

@i-Madsen
Copy link
Contributor Author

Ok, I ran generate_new_playthrough.sh and added the resulting spades.txt - that's the only file needed from this, right?

@lanctot
Copy link
Collaborator

lanctot commented May 24, 2024

Correct, yes. Thanks!

@lanctot
Copy link
Collaborator

lanctot commented May 25, 2024

Seems like there's a seg fault, there might be a memory issue (buffer overrun or similar). Try building in debug mode (see the flag in the main CMakeLists.txt) and running the test under valgrind or gdb, it should catch memory out-of-bound issues.

@i-Madsen
Copy link
Contributor Author

@lanctot Going to run some more tests, but after setting build mode to Debug and running build_and_run_tests.sh, it shows a SEGFAULT error with the dou dizhu test:

image

(If I run with --test_only=spades everything passes.)

@i-Madsen
Copy link
Contributor Author

If this is helpful at all, this is from running dou_dizhu_test in gdb:

image

@lanctot
Copy link
Collaborator

lanctot commented May 28, 2024

Huh, we'll that's interesting. Ok let me do some checks on my end and get back to you.

(Actually I can even do those checks with the Spade implementation imported.)

I'm quite loaded at the moment though, so it might be a few days.

@lanctot lanctot added the imported This PR has been imported and awaiting internal review. Please avoid any more local changes, thanks! label May 31, 2024
@lanctot
Copy link
Collaborator

lanctot commented May 31, 2024

Hmm, ok I did some memory sanitizing checks and there appear to be no problems with Spades nor Dou Dizhu. Thinking this is a fluke. Sometimes github CI has random issues. I've imported it and will push to github momentarily. I expect it'll be fine. Thanks again for the great addition! And will nicely slip right into the repos for the release next week.

Edit: can't explain dou_dizhu failing with debug build on your end, though. Worked fine in debug build + memory sanitizer checks on my side.

@lanctot lanctot merged commit 82b5aac into google-deepmind:master May 31, 2024
9 of 12 checks passed
@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 May 31, 2024
@i-Madsen
Copy link
Contributor Author

Cool! I'll make another PR eventually as I've been adding game specific functions and tweaking the code a bit to deal with a adding a reward bonus for winning to the returns and such, but glad to have a working version in for now!

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