Add Hanabi seat order layout - #190
Open
ravihammond wants to merge 9 commits into
Open
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces a “seat order” concept for the Hanabi environment by adding a shuffle_player_order option (defaulting to True) and exposing a seat_order array on the game State, with accompanying tests intended to validate determinism and legacy behavior.
Changes:
- Add
shuffle_player_orderparameter toHanabiEnv/HanabiGameand persist it on the game. - Extend
Statewithseat_orderand thread it through reset paths. - Add tests for legacy deck mapping when
shuffle_player_order=Falseand for deterministicseat_orderwhen shuffling.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
tests/hanabi/test_hanabi.py |
Adds tests around legacy deck behavior, deterministic shuffling, and deck injection seat order. |
jaxmarl/environments/hanabi/hanabi.py |
Exposes shuffle_player_order on the environment constructor and forwards it to the game. |
jaxmarl/environments/hanabi/hanabi_game.py |
Adds State.seat_order and generates it during reset when shuffling is enabled. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Collaborator
|
sounds good, gonna wait until #186 is in before looking at this :) |
Resolve conflicts from the jaxmarl-2.0 BaseState refactor: - State now subclasses BaseState; seat_order added alongside deck - get_first_state takes jax.Array types - step_env reads from the renamed act_batch Also fix seat/agent index conflation that seat shuffling exposes: - render() indexed self.agents with a seat index - three tests compared cur_player_idx (seat-indexed) to agent list positions; test_noop_is_legal_only_for_non_acting_player failed under a non-identity permutation
shuffle_player_order now defaults to False, so existing Hanabi results and baselines are unaffected. Shuffling is opt-in for the ad-hoc coordination setting it is intended for; the shipped baselines are shared-parameter self-play, where permuting agent-to-seat assignment is a no-op in distribution. Both manual game scripts derived the current player from cur_player_idx, which is seat-indexed, then used it to index agent-ordered legal moves and action arrays. Under a non-identity seat order this selected the wrong player: the human was offered the non-acting agent's move list, whose only legal move is noop. Map through seat_order at the point of derivation, which is the only place either script conflates the two. Tests that assert shuffling behaviour now request it explicitly.
The multi-player test now runs with shuffling both off and on. Its core assertion is that get_legal_moves and step_env agree on which agent key is acting: exactly the acting agent is barred from noop, and the move it sends is executed, passing play to the next seat. Seeding needs care here. The acting seat is always seat 0 at reset, so a permutation that happens to leave agent_0 in that seat exercises nothing even though it is not the identity. Seed 2 seats a different agent in the acting seat for all of 2-5 players, and the test asserts this rather than assuming it, so it cannot quietly stop testing the mapping. Verified by reverting the get_legal_moves remap: all four shuffled cases fail, all four unshuffled cases still pass.
The routing tests asserted on the exact permutation a given PRNG key produced, which is not a stable contract: JAX has changed its threefry defaults before, and the same fragility already forced a fix to tests/smax/test_smax.py::test_obs_function in this release. seat_order only maps seats to agent keys, and cards are dealt to seats independently of it, so a shuffled state can be built by overriding the field. The tests now do that from a fixed table, and a new test pins the assumption behind it: a shuffled reset differs from a fixed one only in seat_order, leaving the deck, hands and current seat untouched. The determinism test keeps the RNG path but no longer asserts which permutation it yields, only that it is one and that it is reproducible.
The multi-player test asserted only that play advanced to the next seat, which a noop does too. Routing the action by seat instead of by agent picks up a non-acting agent's noop, advances the turn, and left the test passing: reverting the step_env mapping failed one test, not five. Compare against the state reached when every agent noops instead. The acting agent's move must produce a different state, which fails as soon as the wrong entry of the action batch is read.
Collaborator
|
@ravihammond a couple small edits including fixing the conflicts caused by #186. I have changed the default behaviour to |
The test played slot 0 and asserted a life was lost, which holds only if the card dealt there is unplayable. Which card that is depends on the JAX PRNG implementation: under jax_threefry_partitionable, which newer JAX defaults to, PRNGKey(42) deals a different hand and the assertion fails. CI installs jax unpinned, so it saw a failure that a local 0.4.37 did not. Inject a deck whose first card is R2 instead. This is the same fix applied to tests/smax/test_smax.py::test_obs_function in this release. Verified with the suite run under both PRNG implementations.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Tests