Refactor implementation of GameTreeRep::IsPerfectRecall, add a new iterator class to zip actions and children; extend tests

Added #include <optional>

Fix clang-tidy

Add ActionsIterator class to zip actions and children; refactor PR-checker using a separate BuildInfosetParents method

Eliminate duplicates in m_infosetParents and improve iterator organization

Author: d-kad

src/games/game.h

@@ -105,6 +105,7 @@ template <class P, class T> class ElementCollection {
       return *this;
     }
     value_type operator*() const { return m_container->at(m_index); }
+    P GetOwner() const { return m_owner; }
   };
 
   ElementCollection() = default;
@@ -458,6 +459,23 @@ class GameNodeRep : public GameObject {
 public:
   using Children = ElementCollection<GameNode, GameNodeRep>;
 
+  /// @brief A range class for iterating over a node's (action, child) pairs.
+  class Actions {
+  private:
+    GameNode m_owner{nullptr};
+
+  public:
+    class iterator;
+
+    explicit Actions(GameNode p_owner);
+
+    iterator begin() const;
+    iterator end() const;
+  };
+
+  /// @brief Returns a range for iterating over this node's (action, child) pairs.
+  Actions GetActions() const;
+
   Game GetGame() const;
 
   const std::string &GetLabel() const { return m_label; }
@@ -488,6 +506,100 @@ class GameNodeRep : public GameObject {
   bool IsSubgameRoot() const;
 };
 
+class GameNodeRep::Actions::iterator {
+public:
+  /// @name Iterator
+  //@{
+  using iterator_category = std::forward_iterator_tag;
+  using value_type = std::pair<GameAction, GameNode>;
+  using difference_type = std::ptrdiff_t;
+  using pointer = value_type *;
+  using reference = value_type;
+  //@}
+
+private:
+  /// @brief An iterator to the action at the parent's information set.
+  GameInfosetRep::Actions::iterator m_action_it;
+  /// @brief An iterator to the child node.
+  GameNodeRep::Children::iterator m_child_it;
+
+public:
+  /// @name Lifecycle
+  //@{
+  /// Default constructor. Creates an iterator in a past-the-end state.
+  iterator() = default;
+
+  /// Creates a new iterator that zips an action iterator and a child iterator.
+  iterator(GameInfosetRep::Actions::iterator p_action_it,
+           GameNodeRep::Children::iterator p_child_it);
+  //   : m_action_it(p_action_it), m_child_it(p_child_it)
+  // {
+  // }
+  //@}
+
+  /// @name Iterator Operations
+  //@{
+  /// Returns the current action-child pair.
+  reference operator*() const { return {*m_action_it, *m_child_it}; }
+
+  /// Advances the iterator to the next pair (pre-increment).
+  iterator &operator++()
+  {
+    ++m_action_it;
+    ++m_child_it;
+    return *this;
+  }
+
+  /// Advances the iterator to the next pair (post-increment).
+  iterator operator++(int)
+  {
+    iterator tmp = *this;
+    ++(*this);
+    return tmp;
+  }
+
+  /// Compares two iterators for equality.
+  bool operator==(const iterator &p_other) const
+  {
+    // Comparing one of the wrapped iterators is sufficient as they move in lockstep.
+    return m_child_it == p_other.m_child_it;
+  }
+
+  /// Compares two iterators for inequality.
+  bool operator!=(const iterator &p_other) const { return !(*this == p_other); }
+  //@}
+
+  GameNode GetOwner() const;
+};
+
+inline GameNodeRep::Actions::Actions(GameNode p_owner) : m_owner(p_owner) {}
+
+inline GameNodeRep::Actions GameNodeRep::GetActions() const { return Actions(this); }
+
+inline GameNodeRep::Actions::iterator GameNodeRep::Actions::begin() const
+{
+  if (m_owner->IsTerminal()) {
+    return end();
+  }
+  return {m_owner->GetInfoset()->GetActions().begin(), m_owner->GetChildren().begin()};
+}
+
+inline GameNodeRep::Actions::iterator GameNodeRep::Actions::end() const
+{
+  if (m_owner->IsTerminal()) {
+    return {};
+  }
+  return {m_owner->GetInfoset()->GetActions().end(), m_owner->GetChildren().end()};
+}
+
+inline GameNodeRep::Actions::iterator::iterator(GameInfosetRep::Actions::iterator p_action_it,
+                                                GameNodeRep::Children::iterator p_child_it)
+  : m_action_it(p_action_it), m_child_it(p_child_it)
+{
+}
+
+inline GameNode GameNodeRep::Actions::iterator::GetOwner() const { return m_child_it.GetOwner(); }
+
 /// This is the class for representing an arbitrary finite game.
 class GameRep : public std::enable_shared_from_this<GameRep> {
   friend class GameOutcomeRep;
@@ -648,15 +760,8 @@ class GameRep : public std::enable_shared_from_this<GameRep> {
   /// Returns the set of terminal nodes which are descendants of members of an action
   virtual std::vector<GameNode> GetPlays(GameAction action) const { throw UndefinedException(); }
 
-  /// Returns true if the game is perfect recall.  If not,
-  /// a pair of violating information sets is returned in the parameters.
-  virtual bool IsPerfectRecall(GameInfoset &, GameInfoset &) const = 0;
   /// Returns true if the game is perfect recall
-  bool IsPerfectRecall() const
-  {
-    GameInfoset s, t;
-    return IsPerfectRecall(s, t);
-  }
+  virtual bool IsPerfectRecall() const = 0;
   //@}
 
   /// @name Writing data files

src/games/gameagg.h

@@ -84,7 +84,7 @@ class GameAGGRep : public GameRep {
   //@{
   bool IsTree() const override { return false; }
   bool IsAgg() const override { return true; }
-  bool IsPerfectRecall(GameInfoset &, GameInfoset &) const override { return true; }
+  bool IsPerfectRecall() const override { return true; }
   bool IsConstSum() const override;
   /// Returns the smallest payoff to any player in any outcome of the game
   Rational GetMinPayoff() const override { return Rational(aggPtr->getMinPayoff()); }

src/games/gamebagg.h

@@ -91,7 +91,7 @@ class GameBAGGRep : public GameRep {
   //@{
   bool IsTree() const override { return false; }
   virtual bool IsBagg() const { return true; }
-  bool IsPerfectRecall(GameInfoset &, GameInfoset &) const override { return true; }
+  bool IsPerfectRecall() const override { return true; }
   bool IsConstSum() const override { throw UndefinedException(); }
   /// Returns the smallest payoff to any player in any outcome of the game
   Rational GetMinPayoff() const override { return Rational(baggPtr->getMinPayoff()); }

src/games/gametable.h

@@ -57,7 +57,7 @@ class GameTableRep : public GameExplicitRep {
   //@{
   bool IsTree() const override { return false; }
   bool IsConstSum() const override;
-  bool IsPerfectRecall(GameInfoset &, GameInfoset &) const override { return true; }
+  bool IsPerfectRecall() const override { return true; }
   //@}
 
   /// @name Dimensions of the game

src/games/gametree.cc

@@ -23,6 +23,8 @@
 #include <iostream>
 #include <algorithm>
 #include <numeric>
+#include <stack>
+#include <set>
 
 #include "gambit.h"
 #include "gametree.h"
@@ -732,54 +734,20 @@ bool GameTreeRep::IsConstSum() const
   }
 }
 
-bool GameTreeRep::IsPerfectRecall(GameInfoset &s1, GameInfoset &s2) const
+bool GameTreeRep::IsPerfectRecall() const
 {
-  for (auto player : m_players) {
-    for (size_t i = 1; i <= player->m_infosets.size(); i++) {
-      auto *iset1 = player->m_infosets[i - 1];
-      for (size_t j = 1; j <= player->m_infosets.size(); j++) {
-        auto *iset2 = player->m_infosets[j - 1];
-
-        bool precedes = false;
-        GameAction action = nullptr;
-
-        for (size_t m = 1; m <= iset2->m_members.size(); m++) {
-          size_t n;
-          for (n = 1; n <= iset1->m_members.size(); n++) {
-            if (iset2->GetMember(m)->IsSuccessorOf(iset1->GetMember(n)) &&
-                iset1->GetMember(n) != iset2->GetMember(m)) {
-              precedes = true;
-              for (const auto &act : iset1->GetActions()) {
-                if (iset2->GetMember(m)->IsSuccessorOf(iset1->GetMember(n)->GetChild(act))) {
-                  if (action != nullptr && action != act) {
-                    s1 = iset1;
-                    s2 = iset2;
-                    return false;
-                  }
-                  action = act;
-                }
-              }
-              break;
-            }
-          }
-
-          if (i == j && precedes) {
-            s1 = iset1;
-            s2 = iset2;
-            return false;
-          }
+  if (m_infosetParents.empty() && !GetRoot()->IsTerminal()) {
+    const_cast<GameTreeRep *>(this)->BuildInfosetParents();
+  }
 
-          if (n > iset1->m_members.size() && precedes) {
-            s1 = iset1;
-            s2 = iset2;
-            return false;
-          }
-        }
-      }
-    }
+  if (GetRoot()->IsTerminal()) {
+    return true;
   }
 
-  return true;
+  const bool violation = std::any_of(m_infosetParents.cbegin(), m_infosetParents.cend(),
+                                     [](const auto &pair) { return pair.second.size() > 1; });
+
+  return !violation;
 }
 
 //------------------------------------------------------------------------
@@ -855,6 +823,7 @@ void GameTreeRep::ClearComputedValues() const
     player->m_strategies.clear();
   }
   const_cast<GameTreeRep *>(this)->m_nodePlays.clear();
+  const_cast<GameTreeRep *>(this)->m_infosetParents.clear();
   m_computedValues = false;
 }
 
@@ -893,6 +862,43 @@ std::vector<GameNodeRep *> GameTreeRep::BuildConsistentPlaysRecursiveImpl(GameNo
   return consistent_plays;
 }
 
+void GameTreeRep::BuildInfosetParents()
+{
+  std::map<GamePlayer, std::stack<GameAction>> prior_actions;
+  std::stack<GameNodeRep::Actions::iterator> position;
+
+  for (auto player : GetPlayers()) {
+    prior_actions[player].emplace(nullptr);
+  }
+  prior_actions[GetChance()].emplace(nullptr);
+  prior_actions[GetRoot()->GetPlayer()].emplace(nullptr);
+
+  position.emplace(GetRoot()->GetActions().begin());
+  m_infosetParents[GetRoot()->GetInfoset()].insert(nullptr);
+
+  while (!position.empty()) {
+    GameNodeRep::Actions::iterator &current_it = position.top();
+    const GameNode parent = current_it.GetOwner();
+
+    if (current_it != parent->GetActions().end()) {
+      auto [action, child] = *current_it;
+
+      prior_actions[parent->GetPlayer()].top() = action;
+
+      if (!child->IsTerminal()) {
+        m_infosetParents[child->GetInfoset()].insert(prior_actions[child->GetPlayer()].top());
+        position.emplace(child->GetActions().begin());
+        prior_actions[child->GetPlayer()].emplace(nullptr);
+      }
+      ++current_it;
+    }
+    else {
+      prior_actions[parent->GetPlayer()].pop();
+      position.pop();
+    }
+  }
+}
+
 //------------------------------------------------------------------------
 //                  GameTreeRep: Writing data files
 //------------------------------------------------------------------------

src/games/gametree.h

@@ -39,6 +39,7 @@ class GameTreeRep : public GameExplicitRep {
   std::size_t m_numNodes = 1;
   std::size_t m_numNonterminalNodes = 0;
   std::map<GameNodeRep *, std::vector<GameNodeRep *>> m_nodePlays;
+  std::map<GameInfoset, std::set<GameAction>> m_infosetParents;
 
   /// @name Private auxiliary functions
   //@{
@@ -72,8 +73,7 @@ class GameTreeRep : public GameExplicitRep {
   //@{
   bool IsTree() const override { return true; }
   bool IsConstSum() const override;
-  using GameRep::IsPerfectRecall;
-  bool IsPerfectRecall(GameInfoset &, GameInfoset &) const override;
+  bool IsPerfectRecall() const override;
   /// Turn on or off automatic canonicalization of the game
   void SetCanonicalization(bool p_doCanon) const
   {
@@ -160,6 +160,7 @@ class GameTreeRep : public GameExplicitRep {
 
 private:
   std::vector<GameNodeRep *> BuildConsistentPlaysRecursiveImpl(GameNodeRep *node);
+  void BuildInfosetParents();
 };
 
 template <class T> class TreeMixedStrategyProfileRep : public MixedStrategyProfileRep<T> {

tests/test_extensive.py

@@ -49,14 +49,28 @@ def test_game_add_players_nolabel():
     game.add_player()
 
 
-def test_game_is_perfect_recall():
-    game = games.read_from_file("perfect_recall.efg")
-    assert game.is_perfect_recall
+@pytest.mark.parametrize("game_filename,expected_result", [
+    # Games that have perfect recall
+    ("e01.efg", True),
+    ("e02.efg", True),
+    ("cent3.efg", True),
+    ("poker.efg", True),
+    ("basic_extensive_game.efg", True),
 
-
-def test_game_is_not_perfect_recall():
-    game = games.read_from_file("not_perfect_recall.efg")
-    assert not game.is_perfect_recall
+    # Games that do not have perfect recall
+    ("wichardt.efg", False),  # forgetting past action
+    ("noPR-action-selten-horse.efg", False),  # forgetting past action
+    ("noPR-information-no-deflate.efg", False),  # forgetting past information
+    ("noPR-AM.efg", False),  # absent-mindedness
+    ("noPR-action-AM.efg", False),  # absent-mindedness + forgetting past action
+])
+def test_is_perfect_recall(game_filename: str, expected_result: bool):
+    """
+    Verify the IsPerfectRecall implementation against a suite of games
+    with and without the perfect recall property.
+    """
+    game = games.read_from_file(game_filename)
+    assert game.is_perfect_recall == expected_result
 
 
 def test_getting_payoff_by_label_string():
@@ -95,7 +109,7 @@ def test_outcome_index_exception_label():
     "game,strategy_labels,np_arrays_of_rsf",
     [
         ###############################################################################
-        # # 1 player; reduction; generic payoffs
+        # 1 player; reduction; generic payoffs
         (
             games.create_reduction_one_player_generic_payoffs_efg(),
             [["11", "12", "2*", "3*", "4*"]],

tests/test_games/noPR-AM.efg

@@ -0,0 +1,13 @@
+EFG 2 R "Untitled Extensive Game" { "Player 1" "Player 2" }
+""
+
+p "" 1 1 "" { "1" "2" } 0
+p "" 2 1 "" { "1" "2" } 0
+p "" 1 1 "" { "1" "2" } 0
+t "" 1 "Outcome 1" { 1, 1 }
+p "" 2 2 "" { "1" "2" "3" } 0
+t "" 2 "Outcome 2" { 0, 2 }
+t "" 5 "Outcome 5" { 0, 5 }
+t "" 6 "Outcome 6" { 0, 6 }
+t "" 3 "Outcome 3" { 0, 3 }
+t "" 4 "Outcome 4" { 2, 0 }