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

[Shogi] Randomly initialize the current_player variable. #1298

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

KazukiOhta
Copy link
Contributor

@KazukiOhta KazukiOhta commented Feb 6, 2025

Thank you for developing your nice library. I am really enjoying using it.

Issue

In Shogi environment, current_player is not randomly initialized.
This can cause issues when the users use the information of current_player in their code.

The following code can reproduce this issue.

import pgx
import jax

env = pgx.make("chess")
chess_states = jax.vmap(env.init)(jax.random.split(jax.random.key(0), 16))
print(chess_states.current_player) # -> [0 0 1 1 1 0 1 1 1 0 1 0 0 0 1 0]

env = pgx.make("shogi")
shogi_states = jax.vmap(env.init)(jax.random.split(jax.random.key(0), 16))
print(shogi_states.current_player) # -> [0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0]

Reason

In the _init function of pgx/pgx/shogi.py, the variable player_order is randomly initialized, but it is not reflected to the variable current_player.

Therefore, current_player stays to be jnp.int32(0), as initialized in class definition.

Proposal

I propose to initialize the current_player variable in the _init function.

Specifically, I propose to copy and paste the _init function in chess.py into shogi.py.

@KazukiOhta
Copy link
Contributor Author

KazukiOhta commented Feb 6, 2025

Hmm, the test on 3.13 has failed.
However, when I look into the test log, many tests are failing such as tests/test_sparrow_mahjong.py, tests/test_play2048.py, tests/test_go.py, ....

Maybe these failures are not caused by the modification in this pull request...?

@sotetsuk
Copy link
Owner

sotetsuk commented Feb 7, 2025

Thank you again for reporting bugs! Recent changes may have introduced this bug ... 😢
I'll check it this weekend!

@KazukiOhta
Copy link
Contributor Author

Thank you for your kind and quick reply!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants