Rename .with_recall to by_last_action and clarify that last action for a player is discriminated by information set

doc/tutorials/advanced_tutorials/h_selector_prototype.ipynb

@@ -106,7 +106,7 @@
     "(call/fold after checking then facing a bet) must still distinguish her own card, even\n",
     "though the tree has grown well past where that distinction was first established.\n",
     "\n",
-    "`alice_partition` is built once, tagged `.with_recall(\"Alice\")`, and reused for both of her\n",
+    "`alice_partition` is built once, tagged `.by_last_action(\"Alice\")`, and reused for both of her\n",
     "decisions -- the tag makes `.plays` automatically fold her own last action into the group\n",
     "key from her second decision onward, with no separate re-derivation step. `bob_partition`\n",
     "never needs the tag: his two uses are his *one* decision instantiated on two mutually\n",
@@ -132,7 +132,7 @@
     }
    },
    "outputs": [],
-   "source": "CARD_VALUE = {\"J\": 0, \"Q\": 1, \"K\": 2}\ncards = list(CARD_VALUE)\n\ng = gbt.ExtensiveGame(players=[\"Alice\", \"Bob\"], title=\"Kuhn poker\")\ng.append_event(H.path(), dict.fromkeys(cards, gbt.Rational(1, 3)))\nfor c in cards:\n    remaining = [x for x in cards if x != c]\n    g.append_event(H.path(c), dict.fromkeys(remaining, gbt.Rational(1, 2)))\n\nalice_partition = H.path(...).by(lambda h: h[0].action).with_recall(\"Alice\")\ng.append_move(alice_partition.plays, \"Alice\", [\"Check\", \"Bet\"])\n\nbob_partition = H.path(..., ...).by(lambda h: h[1].action)\ng.append_move(bob_partition.plays.after(\"Check\"), \"Bob\", [\"Check\", \"Bet\"])\n\ng.append_move(alice_partition.plays.after(\"Check\", \"Bet\"), \"Alice\", [\"Fold\", \"Call\"])\ng.append_move(bob_partition.plays.after(\"Bet\"), \"Bob\", [\"Fold\", \"Call\"])\n\ndraw(g)\nprint(\"is_perfect_recall:\", g.is_perfect_recall)"
+   "source": "CARD_VALUE = {\"J\": 0, \"Q\": 1, \"K\": 2}\ncards = list(CARD_VALUE)\n\ng = gbt.ExtensiveGame(players=[\"Alice\", \"Bob\"], title=\"Kuhn poker\")\ng.append_event(H.path(), dict.fromkeys(cards, gbt.Rational(1, 3)))\nfor c in cards:\n    remaining = [x for x in cards if x != c]\n    g.append_event(H.path(c), dict.fromkeys(remaining, gbt.Rational(1, 2)))\n\nalice_partition = H.path(...).by(lambda h: h[0].action).by_last_action(\"Alice\")\ng.append_move(alice_partition.plays, \"Alice\", [\"Check\", \"Bet\"])\n\nbob_partition = H.path(..., ...).by(lambda h: h[1].action)\ng.append_move(bob_partition.plays.after(\"Check\"), \"Bob\", [\"Check\", \"Bet\"])\n\ng.append_move(alice_partition.plays.after(\"Check\", \"Bet\"), \"Alice\", [\"Fold\", \"Call\"])\ng.append_move(bob_partition.plays.after(\"Bet\"), \"Bob\", [\"Fold\", \"Call\"])\n\ndraw(g)\nprint(\"is_perfect_recall:\", g.is_perfect_recall)"
   },
   {
    "cell_type": "code",
@@ -160,7 +160,7 @@
     "`contrib/games/bayes2a.efg` in the repository. Both players privately learn a type, then\n",
     "move simultaneously each round; each round's actions become public before the next round.\n",
     "\n",
-    "Unlike Kuhn poker, this game needs neither `.with_recall` nor `append_infoset` -- every\n",
+    "Unlike Kuhn poker, this game needs neither `.by_last_action` nor `append_infoset` -- every\n",
     "player's decision falls at a fixed, predictable position in the history across every\n",
     "branch, so plain positional indexing on the augmented history object is all the grouping\n",
     "needs. This is deliberately included as a contrast case: `H`'s dedicated recall machinery\n",

src/pygambit/gametree.pxi

@@ -441,9 +441,9 @@ class ExtensiveGame(Game):
         Applies `grouped`'s initial partition (`base`/`key`), then its
         `post_ops` in order, each one per-group -- expanding/filtering each
         group's own members independently, leaving the key untouched, except
-        that a `.plays` step refines the key by `recall_player`'s last action
-        at that point, if `with_recall` set one (see `GroupedSelector`'s
-        docstring for why).
+        that a `.plays` step refines the key by `last_action_player`'s last
+        action at that point, if `.by_last_action` set one (see
+        `GroupedSelector`'s docstring for why).
         """
         result: dict = {}
         for node in self._get_nodes(grouped.base):
@@ -457,11 +457,13 @@ class ExtensiveGame(Game):
                     expanded = [
                         play for node in nodes for play in cython.cast(Node, node)._plays()
                     ]
-                    if grouped.recall_player is None:
+                    if grouped.last_action_player is None:
                         next_result[key] = expanded
                     else:
                         for play in expanded:
-                            refined_key = (key, _last_action(play, grouped.recall_player))
+                            refined_key = (
+                                key, _last_action_key(play, grouped.last_action_player)
+                            )
                             next_result.setdefault(refined_key, []).append(play)
                     continue
                 if isinstance(op, _AfterStep):

src/pygambit/hsel.pxi

@@ -183,15 +183,25 @@ class GroupedSelector:
     chain onto a plain `Selector`, but apply per-group: each group's own
     members are expanded/filtered independently, and the group's key is left
     untouched -- expanding past a decision point doesn't retroactively change
-    what a group was keyed by. `.with_recall(player)` is the one exception:
-    once set, every subsequent `.plays` on this selector *also* refines each
-    group's key by folding in `player`'s last action at that point, so a
-    partition built for one decision stays a valid recall-respecting
-    partition when reused for a later one, without the caller needing to
-    re-derive or manually re-key it. Scoped to `.plays` specifically for now
-    (not every expand-style op) -- narrower than the full "any expand-style
-    step" idea from the design notes, not yet stress-tested against a shape
-    that would need more.
+    what a group was keyed by. `.by_last_action(player)` is the one
+    exception: once set, every subsequent `.plays` on this selector *also*
+    refines each group's key by folding in `player`'s last action at that
+    point -- keyed on the `(infoset, label)` pair identifying that action,
+    not the label alone, so two infosets that happen to share an action
+    label stay distinct groups -- so a partition built for one decision
+    stays a valid recall-respecting partition when reused for a later one,
+    without the caller needing to re-derive or manually re-key it. Scoped to
+    `.plays` specifically for now (not every expand-style op) -- narrower
+    than the full "any expand-style step" idea from the design notes, not
+    yet stress-tested against a shape that would need more.
+
+    The refinement is relative to this selector chain, not a general recall
+    guarantee: it only folds in the action taken at the `.plays` step
+    immediately before it. A `.plays` that jumps past more than one of
+    `player`'s decisions, or a base partition that already pools together
+    histories differing in an earlier one of `player`'s actions, will
+    silently under-refine -- `.by_last_action` cannot recover information
+    the selector chain already discarded.
 
     .. versionadded:: 17.0.0
     """
@@ -201,35 +211,37 @@ class GroupedSelector:
         base: Selector,
         key: typing.Callable,
         post_ops: tuple = (),
-        recall_player: str = None,
+        last_action_player: str = None,
     ) -> None:
         self.base = base
         self.key = key
         self.post_ops = post_ops
-        self.recall_player = recall_player
+        self.last_action_player = last_action_player
 
     def __repr__(self) -> str:
         return (
             f"GroupedSelector(base={self.base!r}, key={self.key!r}, "
-            f"post_ops={self.post_ops!r}, recall_player={self.recall_player!r})"
+            f"post_ops={self.post_ops!r}, last_action_player={self.last_action_player!r})"
         )
 
     def _extend(self, op) -> GroupedSelector:
-        return GroupedSelector(self.base, self.key, self.post_ops + (op,), self.recall_player)
+        return GroupedSelector(
+            self.base, self.key, self.post_ops + (op,), self.last_action_player
+        )
 
     @property
     def plays(self) -> GroupedSelector:
         """The current terminal frontier of each group, independently --
         see the class docstring for how this interacts with
-        `.with_recall(player)`."""
+        `.by_last_action(player)`."""
         return self._extend(_PlaysStep())
 
     def after(self, *labels: str) -> GroupedSelector:
         """Filter each group to just the members whose own trailing labels
         are exactly `labels`, whatever came before them."""
         return self._extend(_AfterStep(labels))
 
-    def with_recall(self, player: str) -> GroupedSelector:
+    def by_last_action(self, player: str) -> GroupedSelector:
         """From here on, every `.plays` on this selector also refines each
         group's key by folding in `player`'s last action at that point --
         see the class docstring."""
@@ -296,11 +308,12 @@ def _canonical_history_cached(node: Node, cache: dict) -> History:
 
 
 @cython.cfunc
-def _node_last_action(node: c_GameNode, player: str) -> object:
-    """The label of the last action `player` took on the path to `node`
-    (a raw handle, not a `Node`), wherever it fell -- `None` if `player`
-    hasn't acted yet. The `c_GameNode`-only core of `_last_action`, used
-    directly by `HistoryView`, which must never hold a `Node`.
+def _node_last_acted_child(node: c_GameNode, player: str) -> c_GameNode:
+    """Internal: the child (a raw handle, not a `Node`) reached by `player`'s
+    last action on the path to `node` -- i.e. the node whose parent is where
+    `player` acted -- or a null handle if `player` hasn't acted yet. Shared
+    by `_node_last_action` (the action's label) and `_node_last_action_key`
+    (the `(infoset, label)` pair `.by_last_action(player)` keys on).
 
     Raises
     ------
@@ -321,17 +334,68 @@ def _node_last_action(node: c_GameNode, player: str) -> object:
             player_handle != cython.cast(c_GamePlayer, NULL) and
             player_handle.deref().GetLabel().decode("utf-8") == player
         ):
-            return current.deref().GetPriorAction().deref().GetLabel().decode("utf-8")
+            return current
         current = parent
         parent = current.deref().GetParent()
+    return cython.cast(c_GameNode, NULL)
+
+
+@cython.cfunc
+def _node_last_action(node: c_GameNode, player: str) -> object:
+    """The label of the last action `player` took on the path to `node`
+    (a raw handle, not a `Node`), wherever it fell -- `None` if `player`
+    hasn't acted yet. The `c_GameNode`-only core of `_last_action`, used
+    directly by `HistoryView`, which must never hold a `Node`.
+
+    Raises
+    ------
+    KeyError
+        If no player in the game has label `player`.
+    TypeError
+        If `player` is not a `str`.
+    ValueError
+        If `player` is an empty string or all spaces.
+    """
+    child: c_GameNode = _node_last_acted_child(node, player)
+    if child != cython.cast(c_GameNode, NULL):
+        return child.deref().GetPriorAction().deref().GetLabel().decode("utf-8")
+    return None
+
+
+@cython.cfunc
+def _node_last_action_key(node: c_GameNode, player: str) -> object:
+    """The `(infoset, label)` pair identifying `player`'s last action on the
+    path to `node` (a raw handle, not a `Node`), wherever it fell -- `None`
+    if `player` hasn't acted yet. `infoset` is the canonical `History` of the
+    infoset at which the action was taken (see `_canonical_history`,
+    matching `HistoryTransition.state`), so two infosets that happen to
+    share an action label are still told apart. Used by
+    `.by_last_action(player)`'s evaluation, via `_last_action_key`.
+
+    Raises
+    ------
+    KeyError
+        If no player in the game has label `player`.
+    TypeError
+        If `player` is not a `str`.
+    ValueError
+        If `player` is an empty string or all spaces.
+    """
+    child: c_GameNode = _node_last_acted_child(node, player)
+    if child != cython.cast(c_GameNode, NULL):
+        return (
+            _canonical_history(Node.wrap(child.deref().GetParent())),
+            child.deref().GetPriorAction().deref().GetLabel().decode("utf-8"),
+        )
     return None
 
 
-def _last_action(node: Node, player: str) -> str | None:
-    """The label of the last action `player` took on the path to `node`,
-    wherever it fell -- `None` if `player` hasn't acted yet. Used by
-    `.with_recall(player)`'s evaluation, which still works in terms of `Node`."""
-    return _node_last_action(cython.cast(Node, node).node, player)
+def _last_action_key(node: Node, player: str) -> tuple | None:
+    """The `(infoset, label)` pair identifying `player`'s last action on the
+    path to `node`, wherever it fell -- `None` if `player` hasn't acted yet.
+    Used by `.by_last_action(player)`'s evaluation, which still works in
+    terms of `Node`."""
+    return _node_last_action_key(cython.cast(Node, node).node, player)
 
 
 @cython.cclass

tests/test_hsel.py

@@ -156,24 +156,46 @@ def key(h):
     game._get_groups(gbt.H.plays.by(key))
 
 
-def test_with_recall_raises_on_invalid_player():
+def test_by_last_action_raises_on_invalid_player():
     game = gbt.ExtensiveGame(players=["A", "B"])
     game.append_move(gbt.H.path(), "A", ["U", "D"])
     game.append_move(gbt.H.plays, "B", ["x", "y"])
 
-    grouped = gbt.H.path().by(lambda h: None).with_recall("NoSuchPlayer").plays
+    grouped = gbt.H.path().by(lambda h: None).by_last_action("NoSuchPlayer").plays
     with pytest.raises(KeyError):
         game._get_groups(grouped)
 
 
-def test_with_recall_refines_key_by_last_action():
+def test_by_last_action_refines_key_by_last_action():
     game = gbt.ExtensiveGame(players=["A", "B"])
     game.append_move(gbt.H.path(), "A", ["U", "D"])
     game.append_move(gbt.H.plays, "B", ["x", "y"])
 
-    grouped = gbt.H.path().by(lambda h: None).with_recall("A").plays
+    grouped = gbt.H.path().by(lambda h: None).by_last_action("A").plays
     groups = game._get_groups(grouped)
-    assert {k: [h.actions for h in v] for k, v in groups.items()} == {
-        (None, "U"): [("U", "x"), ("U", "y")],
-        (None, "D"): [("D", "x"), ("D", "y")],
+    keyed_by_action = {k[1]: [h.actions for h in v] for k, v in groups.items()}
+    assert {label: histories for (_, label), histories in keyed_by_action.items()} == {
+        "U": [("U", "x"), ("U", "y")],
+        "D": [("D", "x"), ("D", "y")],
+    }
+
+
+def test_by_last_action_distinguishes_infosets_sharing_a_label():
+    # A acts at two distinct infosets (one per branch of the chance move), both
+    # offering an action labelled "U". Grouping only by the label would wrongly
+    # merge these into one group, breaking perfect recall by construction --
+    # `.by_last_action` must key on the (infoset, label) pair instead.
+    game = gbt.ExtensiveGame(players=["A", "B"])
+    game.append_event(gbt.H.path(), {"L": gbt.Rational(1, 2), "R": gbt.Rational(1, 2)})
+    game.append_move(gbt.H.path("L"), "A", ["U", "D"])
+    game.append_move(gbt.H.path("R"), "A", ["U", "D"])
+
+    grouped = gbt.H.path(...).by(lambda h: None).by_last_action("A").plays
+    groups = game._get_groups(grouped)
+    assert len(groups) == 4
+    assert {frozenset(h.actions for h in v) for v in groups.values()} == {
+        frozenset({("L", "U")}),
+        frozenset({("L", "D")}),
+        frozenset({("R", "U")}),
+        frozenset({("R", "D")}),
     }