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

Bridge #1118

Merged
merged 5 commits into from
Oct 5, 2023
Merged

Bridge #1118

merged 5 commits into from
Oct 5, 2023

Conversation

ZiggerZZ
Copy link
Contributor

Hello!

This is the implementation of #1117. It is not ready to be merged yet - I need to change all binvecs in bridge(use_double_dummy_result=false).txt to reflect the change of the order of the tricks (from [previous, current] to [current, previous]). As suggested by @elkhrt, I didn't introduce absolute_order parameter.
If this approach is fine with you, I'll update the tests and the PR will be ready to be merged.

@ZiggerZZ
Copy link
Contributor Author

Fixed the tests. Since I swap the current and the previous tricks, I needed to swap those in the binary representations.
Here's the script that takes bridge(use_double_dummy_result=false).txt and modifies hex values for state observations only for the playing phase (hence condition idx > 700). I used this script to patch the tests.

def change_order(hex_value):
    int_value = int(hex_value, 16)  # Convert the hex string to an integer
    binary_str = bin(int_value)[2:]  # Convert the integer to binary and remove the '0b' prefix
    length_required = 571
    binary_str_padded = binary_str.zfill(length_required)
    index_previous_start, index_previous_end = 129, 337
    index_current_start, index_current_end = 337, 545
    new_string_binary = binary_str_padded[:index_previous_start] + binary_str_padded[index_current_start:index_current_end] + binary_str_padded[index_previous_start: index_previous_end] + binary_str_padded[index_current_end:]
    int_value_from_binary = int(new_string_binary, 2)
    hex_str = hex(int_value_from_binary)
    return hex_str

file_path = "./bridge(use_double_dummy_result=false).txt"

with open(file_path, 'r') as file:
    lines = file.readlines()

for idx, line in enumerate(lines):
	# only apply to lines after bidding
    if "binvec" in line and idx > 700:
        hex_value = line.split(', ')[-1].split(')')[0]  # Extract the hex value
        new_hex_value = change_order(hex_value)
        lines[idx] = line.replace(hex_value, new_hex_value)  # Replace old hex value with the new one

with open(file_path, 'w') as file:
    file.writelines(lines)

@ZiggerZZ ZiggerZZ mentioned this pull request Sep 21, 2023
open_spiel/games/bridge/bridge.cc Outdated Show resolved Hide resolved
open_spiel/games/bridge/bridge.h Outdated Show resolved Hide resolved
@ZiggerZZ
Copy link
Contributor Author

Taken @MattX's review into account

@lanctot
Copy link
Collaborator

lanctot commented Sep 29, 2023

Thanks for this and to @MattX for the review.

We should be able to pull this in soon. I'll just quickly run it by Ed early next week.

@lanctot
Copy link
Collaborator

lanctot commented Sep 29, 2023

BTW @ZiggerZZ, you can always simply regenerate those playthroughs given the new implementation using this script: https://github.com/google-deepmind/open_spiel/blob/master/open_spiel/scripts/regenerate_playthroughs.sh

@ZiggerZZ
Copy link
Contributor Author

Thanks @lanctot! I wanted to verify the correctness of the implementation by writing that logic of modifying the existing observation tensors.

@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 Oct 3, 2023
@lanctot lanctot merged commit 3ad62f6 into google-deepmind:master Oct 5, 2023
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.

3 participants