From 2641fdef1d9bc78076aecedde1dc8bfd09913512 Mon Sep 17 00:00:00 2001 From: Theodore Turocy Date: Fri, 5 Dec 2025 12:30:34 +0000 Subject: [PATCH 01/19] Implement non-copying version of Infosets collection This develops a template that allows us to treat "nested" containers (such as information sets within players) as a single flattened container. As an implication, `GameRep::GetInfosets` now does not create an explicit array of information sets but rather iterates and generates `GameInfoset` handle instances only on demand. Closes #520. --- src/games/game.h | 5 +- src/games/gameobject.h | 118 +++++++++++++++++++++++++++++++++++++++++ src/games/gametree.cc | 9 ---- src/games/gametree.h | 2 +- 4 files changed, 123 insertions(+), 11 deletions(-) diff --git a/src/games/game.h b/src/games/game.h index 2c3f5579c..11804bc58 100644 --- a/src/games/game.h +++ b/src/games/game.h @@ -901,10 +901,13 @@ class GameRep : public std::enable_shared_from_this { /// @name Information sets //@{ + using Infosets = + NestedElementCollection<&GameRep::m_players, &GamePlayerRep::m_infosets, GameInfoset>; + /// Returns the iset'th information set in the game (numbered globally) virtual GameInfoset GetInfoset(int iset) const { throw UndefinedException(); } /// Returns the set of information sets in the game - virtual std::vector GetInfosets() const { throw UndefinedException(); } + virtual Infosets GetInfosets() const { throw UndefinedException(); } /// Sort the information sets for each player in a canonical order virtual void SortInfosets() {} /// Returns the set of actions taken by the infoset's owner before reaching this infoset diff --git a/src/games/gameobject.h b/src/games/gameobject.h index a8328fe3c..c8c608a31 100644 --- a/src/games/gameobject.h +++ b/src/games/gameobject.h @@ -214,6 +214,124 @@ template class ElementCollection { iterator cend() const { return {m_owner, m_container, (m_owner) ? m_container->size() : 0}; } }; +template class NestedElementCollection { + template struct outer_element_of; + + template + struct outer_element_of> OuterClass::*> { + using outer_class = OuterClass; + using outer_elem = Elem; + using container = std::vector>; + }; + + template struct inner_element_of; + + template + struct inner_element_of> OuterElem::*> { + using outer_elem = OuterElem; + using inner_elem = InnerElem; + using container = std::vector>; + }; + + using OM = outer_element_of; + using IM = inner_element_of; + + using OuterClass = typename OM::outer_class; + using OuterElem = typename OM::outer_elem; + using InnerElem = typename IM::inner_elem; + +public: + class iterator { + using outer_iter = typename OM::container::const_iterator; + using inner_iter = typename IM::container::const_iterator; + + outer_iter m_outer, m_outerEnd; + inner_iter m_inner, m_innerEnd; + mutable ValueType m_cache; + + void advance() + { + while (m_outer != m_outerEnd && m_inner == m_innerEnd) { + ++m_outer; + if (m_outer != m_outerEnd) { + const auto &outerPtr = *m_outer; + const auto &innerVec = outerPtr.get()->*InnerMember; + m_inner = innerVec.begin(); + m_innerEnd = innerVec.end(); + } + } + } + + public: + using iterator_category = std::input_iterator_tag; + using value_type = ValueType; + using difference_type = std::ptrdiff_t; + using reference = ValueType; + using pointer = const ValueType *; + + iterator() = default; + + iterator(const OuterClass *p_obj, bool p_isEnd) + { + const auto &outerVec = p_obj->*OuterMember; + + m_outer = outerVec.begin(); + m_outerEnd = outerVec.end(); + + if (p_isEnd || m_outer == m_outerEnd) { + m_outer = m_outerEnd; + return; + } + + const auto &outerPtr = *m_outer; + const auto &innerVec = outerPtr.get()->*InnerMember; + + m_inner = innerVec.begin(); + m_innerEnd = innerVec.end(); + + advance(); + } + + reference operator*() const { return ValueType(*m_inner); } + + pointer operator->() const + { + m_cache = ValueType(*m_inner); + return &m_cache; + } + + iterator &operator++() + { + ++m_inner; + advance(); + return *this; + } + + iterator operator++(int) + { + iterator tmp = *this; + ++(*this); + return tmp; + } + + bool operator==(const iterator &p_other) const + { + return m_outer == p_other.m_outer && (m_outer == m_outerEnd || m_inner == p_other.m_inner); + } + + bool operator!=(const iterator &p_other) const { return !(*this == p_other); } + }; + +private: + const OuterClass *m_obj; + +public: + explicit NestedElementCollection(const OuterClass *p_obj) : m_obj(p_obj) {} + + iterator begin() const { return {m_obj, false}; } + iterator end() const { return {m_obj, true}; } +}; + } // end namespace Gambit #endif // GAMBIT_GAMES_GAMEOBJECT_H diff --git a/src/games/gametree.cc b/src/games/gametree.cc index 6a450fd46..33509228e 100644 --- a/src/games/gametree.cc +++ b/src/games/gametree.cc @@ -1193,15 +1193,6 @@ GameInfoset GameTreeRep::GetInfoset(int p_index) const throw std::out_of_range("Infoset index out of range"); } -std::vector GameTreeRep::GetInfosets() const -{ - std::vector infosets; - for (const auto &player : m_players) { - std::copy(player->m_infosets.begin(), player->m_infosets.end(), std::back_inserter(infosets)); - } - return infosets; -} - //------------------------------------------------------------------------ // GameTreeRep: Outcomes //------------------------------------------------------------------------ diff --git a/src/games/gametree.h b/src/games/gametree.h index f62d26d4b..57e430023 100644 --- a/src/games/gametree.h +++ b/src/games/gametree.h @@ -127,7 +127,7 @@ class GameTreeRep : public GameExplicitRep { /// Returns the iset'th information set in the game (numbered globally) GameInfoset GetInfoset(int iset) const override; /// Returns the set of information sets in the game - std::vector GetInfosets() const override; + Infosets GetInfosets() const override { return Infosets(this); } /// Sort the information sets for each player in a canonical order void SortInfosets() override; /// Returns the set of actions taken by the infoset's owner before reaching this infoset From 56880c4862a48b03be2df538650c9ca5f4c14385 Mon Sep 17 00:00:00 2001 From: Theodore Turocy Date: Fri, 5 Dec 2025 14:22:59 +0000 Subject: [PATCH 02/19] Develop Strategies for game and use in liap solver. --- src/core/util.h | 9 +++++++ src/games/game.h | 9 +++++++ src/solvers/liap/nfgliap.cc | 48 +++++++++++++++++-------------------- 3 files changed, 40 insertions(+), 26 deletions(-) diff --git a/src/core/util.h b/src/core/util.h index 720b1b729..f5678c706 100644 --- a/src/core/util.h +++ b/src/core/util.h @@ -157,6 +157,15 @@ auto minimize_function(const Container &p_container, const Func &p_function) [](const T &a, const T &b) -> T { return std::min(a, b); }, p_function); } +/// @brief Returns the sum of the function over the container +template +auto sum_function(const Container &p_container, const Func &p_function) +{ + using T = decltype(p_function(*(p_container.begin()))); + return std::transform_reduce(p_container.begin(), p_container.end(), static_cast(0), + std::plus<>{}, p_function); +} + //======================================================================== // Exception classes //======================================================================== diff --git a/src/games/game.h b/src/games/game.h index 11804bc58..c85ea353b 100644 --- a/src/games/game.h +++ b/src/games/game.h @@ -820,10 +820,19 @@ class GameRep : public std::enable_shared_from_this { /// @name Dimensions of the game //@{ + using Strategies = + NestedElementCollection<&GameRep::m_players, &GamePlayerRep::m_strategies, GameStrategy>; + /// The number of strategies for each player virtual Array NumStrategies() const = 0; /// Gets the i'th strategy in the game, numbered globally virtual GameStrategy GetStrategy(int p_index) const = 0; + /// Gets the collection of all strategies in the game + Strategies GetStrategies() const + { + BuildComputedValues(); + return Strategies(this); + } /// Creates a new strategy for the player virtual GameStrategy NewStrategy(const GamePlayer &p_player, const std::string &p_label) { diff --git a/src/solvers/liap/nfgliap.cc b/src/solvers/liap/nfgliap.cc index f1fe4a11a..ee65a1a05 100644 --- a/src/solvers/liap/nfgliap.cc +++ b/src/solvers/liap/nfgliap.cc @@ -63,34 +63,32 @@ class StrategicLyapunovFunction : public FunctionOnSimplices { inline double sum_player_probs(const MixedStrategyProfile &p_profile, const GamePlayer &p_player) { - auto strategies = p_player->GetStrategies(); - return std::accumulate( - strategies.cbegin(), strategies.cend(), 0.0, - [p_profile](double t, const GameStrategy &s) { return t + p_profile[s]; }); + return sum_function(p_player->GetStrategies(), + [&p_profile](const auto &s) -> double { return p_profile[s]; }); } double StrategicLyapunovFunction::Value(const Vector &v) const { m_profile = v; double value = 0; - // Liapunov function proper - should be replaced with call to profile once - // the penalty is removed from that implementation. - for (auto player : m_profile.GetGame()->GetPlayers()) { - for (auto strategy : player->GetStrategies()) { - value += sqr( - std::max(m_scale * (m_profile.GetPayoff(strategy) - m_profile.GetPayoff(player)), 0.0)); + // Liapunov function proper + for (const auto &player : m_profile.GetGame()->GetPlayers()) { + const auto payoff = m_profile.GetPayoff(player); + for (const auto strategy : player->GetStrategies()) { + value += sqr(std::max(m_scale * (m_profile.GetPayoff(strategy) - payoff), 0.0)); } } // Penalty function for non-negativity constraint for each strategy - for (auto player : m_profile.GetGame()->GetPlayers()) { - for (auto strategy : player->GetStrategies()) { - value += m_penalty * sqr(std::min(m_profile[strategy], 0.0)); - } - } + value += m_penalty * + sum_function(m_profile.GetGame()->GetStrategies(), [this](const auto &s) -> double { + return sqr(std::min(m_profile[s], 0.0)); + }); + // Penalty function for sum-to-one constraint for each player - for (auto player : m_profile.GetGame()->GetPlayers()) { - value += m_penalty * sqr(sum_player_probs(m_profile, player) - 1.0); - } + value += + m_penalty * sum_function(m_profile.GetGame()->GetPlayers(), [this](const auto &p) -> double { + return sqr(sum_player_probs(m_profile, p) - 1.0); + }); return value; } @@ -98,10 +96,10 @@ double StrategicLyapunovFunction::LiapDerivValue(const MixedStrategyProfileGetPlayers()) { - for (auto strategy : player->GetStrategies()) { - const double loss = - sqr(m_scale) * (p_profile.GetPayoff(strategy) - p_profile.GetPayoff(player)); + for (const auto &player : m_game->GetPlayers()) { + const auto payoff = p_profile.GetPayoff(player); + for (const auto &strategy : player->GetStrategies()) { + const double loss = sqr(m_scale) * (p_profile.GetPayoff(strategy) - payoff); if (loss <= 0.0) { continue; } @@ -137,10 +135,8 @@ namespace { MixedStrategyProfile EnforceNonnegativity(const MixedStrategyProfile &p_profile) { auto profile = p_profile; - for (auto player : p_profile.GetGame()->GetPlayers()) { - for (auto strategy : player->GetStrategies()) { - profile[strategy] = std::max(profile[strategy], 0.0); - } + for (const auto &strategy : p_profile.GetGame()->GetStrategies()) { + profile[strategy] = std::max(profile[strategy], 0.0); } return profile.Normalize(); } From 57b01637064a2562ba630acde8d731c514cf0d4a Mon Sep 17 00:00:00 2001 From: Theodore Turocy Date: Fri, 5 Dec 2025 14:57:37 +0000 Subject: [PATCH 03/19] Increase pytest verbosity to diagnose windows failure --- .github/workflows/python.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/python.yml b/.github/workflows/python.yml index 794340dde..d0eb8f878 100644 --- a/.github/workflows/python.yml +++ b/.github/workflows/python.yml @@ -107,4 +107,4 @@ jobs: run: | python -m pip install -v . - name: Run tests - run: pytest + run: pytest -vv From 7a7686d680b60becb859bca2d57ce3e12ee83d26 Mon Sep 17 00:00:00 2001 From: Theodore Turocy Date: Fri, 5 Dec 2025 15:21:04 +0000 Subject: [PATCH 04/19] temporarily disable crashing test --- tests/test_actions.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/test_actions.py b/tests/test_actions.py index 72cb506a5..782bb19d8 100644 --- a/tests/test_actions.py +++ b/tests/test_actions.py @@ -231,6 +231,7 @@ def test_strategy_action_raises_value_error_for_wrong_player(game, player_ind, i def test_strategy_action_raises_error_for_strategic_game(): """Verify `Strategy.action` retrieves the action prescribed by the strategy """ + return game_efg = games.read_from_file("e02.efg") game_nfg = game_efg.from_arrays(game_efg.to_arrays()[0], game_efg.to_arrays()[1]) alice = game_nfg.players[0] From 7a5cabf1d8438223fe1f221d29b79dab419630b8 Mon Sep 17 00:00:00 2001 From: Theodore Turocy Date: Fri, 5 Dec 2025 15:59:37 +0000 Subject: [PATCH 05/19] Simple non-templated view and iterator. --- src/games/game.h | 77 ++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 74 insertions(+), 3 deletions(-) diff --git a/src/games/game.h b/src/games/game.h index c85ea353b..57476b609 100644 --- a/src/games/game.h +++ b/src/games/game.h @@ -910,13 +910,12 @@ class GameRep : public std::enable_shared_from_this { /// @name Information sets //@{ - using Infosets = - NestedElementCollection<&GameRep::m_players, &GamePlayerRep::m_infosets, GameInfoset>; + class Infosets; /// Returns the iset'th information set in the game (numbered globally) virtual GameInfoset GetInfoset(int iset) const { throw UndefinedException(); } /// Returns the set of information sets in the game - virtual Infosets GetInfosets() const { throw UndefinedException(); } + virtual Infosets GetInfosets() const; /// Sort the information sets for each player in a canonical order virtual void SortInfosets() {} /// Returns the set of actions taken by the infoset's owner before reaching this infoset @@ -968,6 +967,78 @@ class GameRep : public std::enable_shared_from_this { virtual void BuildComputedValues() const {} }; +class GameRep::Infosets { +public: + class iterator { + public: + using iterator_category = std::input_iterator_tag; + using value_type = GameInfoset; + using difference_type = std::ptrdiff_t; + using pointer = GameInfoset; + using reference = GameInfoset; + + using outer_iter = std::vector>::const_iterator; + using inner_iter = std::vector>::const_iterator; + + iterator() = default; + + iterator(outer_iter outer, outer_iter outer_end) : outer_(outer), outer_end_(outer_end) + { + if (outer_ != outer_end_) { + inner_ = (*outer_)->m_infosets.begin(); + inner_end_ = (*outer_)->m_infosets.end(); + satisfy_invariant(); + } + } + + GameInfoset operator*() const { return *inner_; } + GameInfoset operator->() const { return *inner_; } + + iterator &operator++() + { + ++inner_; + satisfy_invariant(); + return *this; + } + + bool operator==(const iterator &other) const + { + return outer_ == other.outer_ && (outer_ == outer_end_ || inner_ == other.inner_); + } + + bool operator!=(const iterator &other) const { return !(*this == other); } + + private: + void satisfy_invariant() + { + while (outer_ != outer_end_ && inner_ == inner_end_) { + ++outer_; + if (outer_ != outer_end_) { + inner_ = (*outer_)->m_infosets.begin(); + inner_end_ = (*outer_)->m_infosets.end(); + } + } + } + + outer_iter outer_{}; + outer_iter outer_end_{}; + inner_iter inner_{}; + inner_iter inner_end_{}; + }; + + // View interface + iterator begin() const { return {players_.begin(), players_.end()}; } + + iterator end() const { return {players_.end(), players_.end()}; } + + explicit Infosets(const GameRep *game) : players_(game->m_players) {} + +private: + const std::vector> &players_; +}; + +inline GameRep::Infosets GameRep::GetInfosets() const { return Infosets(this); } + //======================================================================= // Inline members of game representation classes //======================================================================= From c7293cc16f52bc29c0af5433a18d30d931562ca6 Mon Sep 17 00:00:00 2001 From: Theodore Turocy Date: Fri, 5 Dec 2025 16:14:18 +0000 Subject: [PATCH 06/19] Attempt to Windows-proof common MSVC issues. --- src/games/game.h | 48 ++++++++++++++++++++++++++++++++----------- tests/test_actions.py | 1 - 2 files changed, 36 insertions(+), 13 deletions(-) diff --git a/src/games/game.h b/src/games/game.h index 57476b609..d74005ab0 100644 --- a/src/games/game.h +++ b/src/games/game.h @@ -985,8 +985,7 @@ class GameRep::Infosets { iterator(outer_iter outer, outer_iter outer_end) : outer_(outer), outer_end_(outer_end) { if (outer_ != outer_end_) { - inner_ = (*outer_)->m_infosets.begin(); - inner_end_ = (*outer_)->m_infosets.end(); + init_inner(); satisfy_invariant(); } } @@ -1001,21 +1000,44 @@ class GameRep::Infosets { return *this; } + // -------- Windows/MSVC-safe equality ---------- bool operator==(const iterator &other) const { - return outer_ == other.outer_ && (outer_ == outer_end_ || inner_ == other.inner_); + // Both are end() ⇒ equal + if (is_end() && other.is_end()) { + return true; + } + + // Otherwise, must match outer + inner (safe because both non-end) + return outer_ == other.outer_ && inner_ == other.inner_; } bool operator!=(const iterator &other) const { return !(*this == other); } private: + bool is_end() const { return outer_ == outer_end_; } + + // Safe initialization even when shared_ptr is null + void init_inner() + { + const auto &playerPtr = *outer_; + + if (!playerPtr) { + // Null shared_ptr → no infosets + inner_ = inner_end_ = inner_iter{}; + return; + } + + inner_ = playerPtr->m_infosets.begin(); + inner_end_ = playerPtr->m_infosets.end(); + } + void satisfy_invariant() { - while (outer_ != outer_end_ && inner_ == inner_end_) { + while (!is_end() && inner_ == inner_end_) { ++outer_; - if (outer_ != outer_end_) { - inner_ = (*outer_)->m_infosets.begin(); - inner_end_ = (*outer_)->m_infosets.end(); + if (!is_end()) { + init_inner(); } } } @@ -1027,14 +1049,16 @@ class GameRep::Infosets { }; // View interface - iterator begin() const { return {players_.begin(), players_.end()}; } + iterator begin() const { return iterator(players_->begin(), players_->end()); } + iterator end() const { return iterator(players_->end(), players_->end()); } - iterator end() const { return {players_.end(), players_.end()}; } - - explicit Infosets(const GameRep *game) : players_(game->m_players) {} + explicit Infosets(const GameRep *game) + : players_(&game->m_players) // ✔ store pointer, not reference + { + } private: - const std::vector> &players_; + const std::vector> *players_; }; inline GameRep::Infosets GameRep::GetInfosets() const { return Infosets(this); } diff --git a/tests/test_actions.py b/tests/test_actions.py index 782bb19d8..72cb506a5 100644 --- a/tests/test_actions.py +++ b/tests/test_actions.py @@ -231,7 +231,6 @@ def test_strategy_action_raises_value_error_for_wrong_player(game, player_ind, i def test_strategy_action_raises_error_for_strategic_game(): """Verify `Strategy.action` retrieves the action prescribed by the strategy """ - return game_efg = games.read_from_file("e02.efg") game_nfg = game_efg.from_arrays(game_efg.to_arrays()[0], game_efg.to_arrays()[1]) alice = game_nfg.players[0] From 89d36ddc39feaab94b1fd2a20eb159a19849e46e Mon Sep 17 00:00:00 2001 From: Theodore Turocy Date: Fri, 5 Dec 2025 16:29:48 +0000 Subject: [PATCH 07/19] Extra-strict iterator implementation for debugging --- src/games/game.h | 113 ++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 93 insertions(+), 20 deletions(-) diff --git a/src/games/game.h b/src/games/game.h index d74005ab0..e720cf950 100644 --- a/src/games/game.h +++ b/src/games/game.h @@ -967,71 +967,122 @@ class GameRep : public std::enable_shared_from_this { virtual void BuildComputedValues() const {} }; +#include + class GameRep::Infosets { public: class iterator { public: - using iterator_category = std::input_iterator_tag; + using iterator_category = std::forward_iterator_tag; // diagnostic: proper category using value_type = GameInfoset; using difference_type = std::ptrdiff_t; - using pointer = GameInfoset; - using reference = GameInfoset; + using pointer = value_type *; + using reference = value_type &; using outer_iter = std::vector>::const_iterator; using inner_iter = std::vector>::const_iterator; - iterator() = default; + iterator() : owner_id_(0), players_(nullptr) {} - iterator(outer_iter outer, outer_iter outer_end) : outer_(outer), outer_end_(outer_end) + iterator(outer_iter outer, outer_iter outer_end, + const std::vector> *players, std::size_t owner_id) + : outer_(outer), outer_end_(outer_end), players_(players), owner_id_(owner_id) { + // ------ DIAGNOSTICS ---------------------------------------------------- + + // Detect dangling players pointer + assert(players_ != nullptr && "Infosets iterator constructed with null players pointer."); + + // Detect begin()==end() but outer mismatch + if (outer_ == outer_end_) { + assert(inner_ == inner_end_ && "Inner iterators must be empty for end iterator."); + } + if (outer_ != outer_end_) { init_inner(); satisfy_invariant(); } } - GameInfoset operator*() const { return *inner_; } - GameInfoset operator->() const { return *inner_; } + // ------------------------------------------------------------------------- + // Dereference + // ------------------------------------------------------------------------- + value_type operator*() const + { + assert(players_ != nullptr); + assert(!is_end() && "Dereferencing end iterator!"); + + // Check many possible UB causes + assert(inner_ != inner_end_ && "Inner iterator invalid during dereference."); + assert(valid_owner() && + "Iterator used after view destruction or copying from different view."); + // Additional optional check: ensure GameInfoset is safe to construct + // You can instrument your GameInfoset(value_type) here to assert its invariants. + + return *inner_; + } + + value_type operator->() const { return operator*(); } + + // ------------------------------------------------------------------------- + // Increment + // ------------------------------------------------------------------------- iterator &operator++() { + assert(players_ != nullptr); + assert(!is_end() && "Incrementing end iterator!"); + ++inner_; satisfy_invariant(); return *this; } - // -------- Windows/MSVC-safe equality ---------- + // ------------------------------------------------------------------------- + // Equality / Inequality + // ------------------------------------------------------------------------- bool operator==(const iterator &other) const { - // Both are end() ⇒ equal + // Detect comparing iterators from different views + if (owner_id_ != other.owner_id_) { + assert(false && "Comparing iterators from different Infosets views!"); + } + + // Both end iterators must compare equal if (is_end() && other.is_end()) { return true; } - // Otherwise, must match outer + inner (safe because both non-end) + // Detect illegal comparisons + assert(players_ == other.players_ && + "Comparing iterators belonging to different container instances!"); + return outer_ == other.outer_ && inner_ == other.inner_; } bool operator!=(const iterator &other) const { return !(*this == other); } private: + // ------------------------------------------------------------------------- + // Helpers + // ------------------------------------------------------------------------- bool is_end() const { return outer_ == outer_end_; } - // Safe initialization even when shared_ptr is null + bool valid_owner() const { return players_ != nullptr && owner_id_ != 0; } + + // Initialize inner iter safely void init_inner() { const auto &playerPtr = *outer_; - if (!playerPtr) { - // Null shared_ptr → no infosets - inner_ = inner_end_ = inner_iter{}; - return; - } + // Check for null shared_ptr (VERY COMMON SOURCE OF UB) + assert(playerPtr && "Null shared_ptr inside m_players!"); inner_ = playerPtr->m_infosets.begin(); inner_end_ = playerPtr->m_infosets.end(); } + // Advance to next non-empty inner range void satisfy_invariant() { while (!is_end() && inner_ == inner_end_) { @@ -1042,23 +1093,45 @@ class GameRep::Infosets { } } + // ------------------------------------------------------------------------- + // Data members + // ------------------------------------------------------------------------- outer_iter outer_{}; outer_iter outer_end_{}; inner_iter inner_{}; inner_iter inner_end_{}; + + const std::vector> *players_ = nullptr; + + // Unique stamp to detect view lifetime/dangling iterator + std::size_t owner_id_ = 0; }; + // ------------------------------------------------------------------------- // View interface - iterator begin() const { return iterator(players_->begin(), players_->end()); } - iterator end() const { return iterator(players_->end(), players_->end()); } + // ------------------------------------------------------------------------- + iterator begin() const + { + return iterator(players_->begin(), players_->end(), players_, owner_id_); + } + + iterator end() const { return iterator(players_->end(), players_->end(), players_, owner_id_); } - explicit Infosets(const GameRep *game) - : players_(&game->m_players) // ✔ store pointer, not reference + explicit Infosets(const GameRep *game) : players_(&game->m_players) { + assert(game != nullptr); + owner_id_ = next_owner_id(); // unique ID for detecting illegal comparisons } private: const std::vector> *players_; + std::size_t owner_id_; + + static std::size_t next_owner_id() + { + static std::size_t id = 1; + return id++; + } }; inline GameRep::Infosets GameRep::GetInfosets() const { return Infosets(this); } From 9d55320438f4d406c40751660f457573555367f2 Mon Sep 17 00:00:00 2001 From: Theodore Turocy Date: Fri, 5 Dec 2025 16:49:02 +0000 Subject: [PATCH 08/19] Hold shared pointer to ensure lifetime --- src/games/game.h | 122 +++++++++---------------------------------- src/games/gametree.h | 5 +- 2 files changed, 30 insertions(+), 97 deletions(-) diff --git a/src/games/game.h b/src/games/game.h index e720cf950..6e8f1352e 100644 --- a/src/games/game.h +++ b/src/games/game.h @@ -967,122 +967,66 @@ class GameRep : public std::enable_shared_from_this { virtual void BuildComputedValues() const {} }; -#include - class GameRep::Infosets { public: class iterator { public: - using iterator_category = std::forward_iterator_tag; // diagnostic: proper category + using iterator_category = std::forward_iterator_tag; using value_type = GameInfoset; using difference_type = std::ptrdiff_t; - using pointer = value_type *; - using reference = value_type &; + using pointer = value_type; + using reference = value_type; using outer_iter = std::vector>::const_iterator; using inner_iter = std::vector>::const_iterator; - iterator() : owner_id_(0), players_(nullptr) {} + iterator() = default; - iterator(outer_iter outer, outer_iter outer_end, - const std::vector> *players, std::size_t owner_id) - : outer_(outer), outer_end_(outer_end), players_(players), owner_id_(owner_id) + iterator(std::shared_ptr game, outer_iter outer, outer_iter outer_end) + : game_(std::move(game)), outer_(outer), outer_end_(outer_end) { - // ------ DIAGNOSTICS ---------------------------------------------------- - - // Detect dangling players pointer - assert(players_ != nullptr && "Infosets iterator constructed with null players pointer."); - - // Detect begin()==end() but outer mismatch - if (outer_ == outer_end_) { - assert(inner_ == inner_end_ && "Inner iterators must be empty for end iterator."); - } - if (outer_ != outer_end_) { init_inner(); satisfy_invariant(); } } - // ------------------------------------------------------------------------- - // Dereference - // ------------------------------------------------------------------------- value_type operator*() const { - assert(players_ != nullptr); - assert(!is_end() && "Dereferencing end iterator!"); - - // Check many possible UB causes - assert(inner_ != inner_end_ && "Inner iterator invalid during dereference."); - assert(valid_owner() && - "Iterator used after view destruction or copying from different view."); - - // Additional optional check: ensure GameInfoset is safe to construct - // You can instrument your GameInfoset(value_type) here to assert its invariants. - - return *inner_; + return GameInfoset(*inner_); // GameObjectPtr constructor } - value_type operator->() const { return operator*(); } + value_type operator->() const { return GameInfoset(*inner_); } - // ------------------------------------------------------------------------- - // Increment - // ------------------------------------------------------------------------- iterator &operator++() { - assert(players_ != nullptr); - assert(!is_end() && "Incrementing end iterator!"); - ++inner_; satisfy_invariant(); return *this; } - // ------------------------------------------------------------------------- - // Equality / Inequality - // ------------------------------------------------------------------------- bool operator==(const iterator &other) const { - // Detect comparing iterators from different views - if (owner_id_ != other.owner_id_) { - assert(false && "Comparing iterators from different Infosets views!"); - } - - // Both end iterators must compare equal - if (is_end() && other.is_end()) { + // Compare end iterators correctly + if (outer_ == outer_end_ && other.outer_ == other.outer_end_) { return true; } - // Detect illegal comparisons - assert(players_ == other.players_ && - "Comparing iterators belonging to different container instances!"); - return outer_ == other.outer_ && inner_ == other.inner_; } bool operator!=(const iterator &other) const { return !(*this == other); } private: - // ------------------------------------------------------------------------- - // Helpers - // ------------------------------------------------------------------------- bool is_end() const { return outer_ == outer_end_; } - bool valid_owner() const { return players_ != nullptr && owner_id_ != 0; } - - // Initialize inner iter safely void init_inner() { - const auto &playerPtr = *outer_; - - // Check for null shared_ptr (VERY COMMON SOURCE OF UB) - assert(playerPtr && "Null shared_ptr inside m_players!"); - - inner_ = playerPtr->m_infosets.begin(); - inner_end_ = playerPtr->m_infosets.end(); + const auto &player = *outer_; + inner_ = player->m_infosets.begin(); + inner_end_ = player->m_infosets.end(); } - // Advance to next non-empty inner range void satisfy_invariant() { while (!is_end() && inner_ == inner_end_) { @@ -1093,48 +1037,34 @@ class GameRep::Infosets { } } - // ------------------------------------------------------------------------- - // Data members - // ------------------------------------------------------------------------- + // HOLD GAME ALIVE + std::shared_ptr game_; + outer_iter outer_{}; outer_iter outer_end_{}; inner_iter inner_{}; inner_iter inner_end_{}; - - const std::vector> *players_ = nullptr; - - // Unique stamp to detect view lifetime/dangling iterator - std::size_t owner_id_ = 0; }; - // ------------------------------------------------------------------------- - // View interface - // ------------------------------------------------------------------------- - iterator begin() const + // Construct a view storing a strong reference to the GameRep + explicit Infosets(std::shared_ptr game) + : game_(std::move(game)), players_(&game_->m_players) { - return iterator(players_->begin(), players_->end(), players_, owner_id_); } - iterator end() const { return iterator(players_->end(), players_->end(), players_, owner_id_); } + iterator begin() const { return iterator(game_, players_->begin(), players_->end()); } - explicit Infosets(const GameRep *game) : players_(&game->m_players) - { - assert(game != nullptr); - owner_id_ = next_owner_id(); // unique ID for detecting illegal comparisons - } + iterator end() const { return iterator(game_, players_->end(), players_->end()); } private: + std::shared_ptr game_; const std::vector> *players_; - std::size_t owner_id_; - - static std::size_t next_owner_id() - { - static std::size_t id = 1; - return id++; - } }; -inline GameRep::Infosets GameRep::GetInfosets() const { return Infosets(this); } +inline GameRep::Infosets GameRep::GetInfosets() const +{ + return Infosets(std::const_pointer_cast(this->shared_from_this())); +} //======================================================================= // Inline members of game representation classes diff --git a/src/games/gametree.h b/src/games/gametree.h index 57e430023..93245c34e 100644 --- a/src/games/gametree.h +++ b/src/games/gametree.h @@ -127,7 +127,10 @@ class GameTreeRep : public GameExplicitRep { /// Returns the iset'th information set in the game (numbered globally) GameInfoset GetInfoset(int iset) const override; /// Returns the set of information sets in the game - Infosets GetInfosets() const override { return Infosets(this); } + Infosets GetInfosets() const override + { + return Infosets(std::const_pointer_cast(this->shared_from_this())); + } /// Sort the information sets for each player in a canonical order void SortInfosets() override; /// Returns the set of actions taken by the infoset's owner before reaching this infoset From 7f7d1d595f0077a2232362833a7a60d25af624f0 Mon Sep 17 00:00:00 2001 From: Theodore Turocy Date: Fri, 5 Dec 2025 17:19:49 +0000 Subject: [PATCH 09/19] Experiment with two-phase construction --- src/games/gametable.cc | 16 +++++++++++++++- src/games/gametable.h | 6 +++++- src/games/gametree.cc | 26 ++++++++++++++++++++++---- src/games/gametree.h | 6 +++++- 4 files changed, 47 insertions(+), 7 deletions(-) diff --git a/src/games/gametable.cc b/src/games/gametable.cc index 252e5790e..66bdfcbf9 100644 --- a/src/games/gametable.cc +++ b/src/games/gametable.cc @@ -21,6 +21,7 @@ // #include +#include #include "gambit.h" #include "gametable.h" @@ -66,7 +67,7 @@ std::shared_ptr TablePureStrategyProfileRep::Copy() cons Game NewTable(const std::vector &p_dim, bool p_sparseOutcomes /*= false*/) { - return std::make_shared(p_dim, p_sparseOutcomes); + return GameTableRep::Create(p_dim, p_sparseOutcomes); } //------------------------------------------------------------------------ @@ -263,6 +264,10 @@ template class TableMixedStrategyProfileRep; GameTableRep::GameTableRep(const std::vector &dim, bool p_sparseOutcomes /* = false */) : m_results(std::accumulate(dim.begin(), dim.end(), 1, std::multiplies<>())) +{ +} + +void GameTableRep::Initialize(const std::vector &dim, bool p_sparseOutcomes /* = false */) { for (const auto &nstrat : dim) { m_players.push_back(std::make_shared(this, m_players.size() + 1, nstrat)); @@ -287,6 +292,15 @@ GameTableRep::GameTableRep(const std::vector &dim, bool p_sparseOutcomes /* } } +std::shared_ptr GameTableRep::Create(const std::vector &dim, + bool sparseOutcomes) +{ + auto rep = std::shared_ptr(new GameTableRep(dim, sparseOutcomes)); + rep->Initialize(dim, sparseOutcomes); + + return rep; +} + Game GameTableRep::Copy() const { std::ostringstream os; diff --git a/src/games/gametable.h b/src/games/gametable.h index 3aab1dd38..193c58173 100644 --- a/src/games/gametable.h +++ b/src/games/gametable.h @@ -44,12 +44,16 @@ class GameTableRep : public GameExplicitRep { void RebuildTable(); //@} + explicit GameTableRep(const std::vector &p_dim, bool p_sparseOutcomes = false); + void Initialize(const std::vector &dim, bool p_sparseOutcomes); + public: /// @name Lifecycle //@{ /// Construct a new table game with the given dimension /// If p_sparseOutcomes = true, outcomes for all contingencies are left null - explicit GameTableRep(const std::vector &p_dim, bool p_sparseOutcomes = false); + static std::shared_ptr Create(const std::vector &dim, + bool sparseOutcomes = false); Game Copy() const override; //@} diff --git a/src/games/gametree.cc b/src/games/gametree.cc index 33509228e..0c781be07 100644 --- a/src/games/gametree.cc +++ b/src/games/gametree.cc @@ -701,10 +701,28 @@ GameInfoset GameTreeRep::InsertMove(GameNode p_node, GameInfoset p_infoset) // GameTreeRep: Lifecycle //------------------------------------------------------------------------ -GameTreeRep::GameTreeRep() - : m_root(std::make_shared(this, nullptr)), - m_chance(std::make_shared(this, 0)) +GameTreeRep::GameTreeRep() = default; + +void GameTreeRep::Initialize() +{ + auto self = shared_from_this(); + + // Now construct children using the VALID GameTreeRep* + m_root = std::make_shared(self.get(), nullptr); + m_chance = std::make_shared(self.get(), 0); + + // If GameNodeRep creates children of its own, same pattern applies +} + +std::shared_ptr GameTreeRep::Create() { + // Construct object under shared_ptr CONTROL BLOCK + auto rep = std::shared_ptr(new GameTreeRep()); + + // Now safe to construct children because shared_from_this works + rep->Initialize(); + + return rep; } GameTreeRep::~GameTreeRep() @@ -721,7 +739,7 @@ Game GameTreeRep::Copy() const return ReadGame(is); } -Game NewTree() { return std::make_shared(); } +Game NewTree() { return GameTreeRep::Create(); } //------------------------------------------------------------------------ // GameTreeRep: General data access diff --git a/src/games/gametree.h b/src/games/gametree.h index 93245c34e..cb88b9168 100644 --- a/src/games/gametree.h +++ b/src/games/gametree.h @@ -68,10 +68,14 @@ class GameTreeRep : public GameExplicitRep { void CopySubtree(GameNodeRep *, GameNodeRep *, GameNodeRep *); //@} + GameTreeRep(); + void Initialize(); + public: /// @name Lifecycle //@{ - GameTreeRep(); + + static std::shared_ptr Create(); ~GameTreeRep() override; Game Copy() const override; //@} From 23f293a9141f6854d4b32c330ffdb63e398ea47c Mon Sep 17 00:00:00 2001 From: Theodore Turocy Date: Fri, 5 Dec 2025 17:30:23 +0000 Subject: [PATCH 10/19] More debug assertions --- src/games/game.h | 23 ++++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-) diff --git a/src/games/game.h b/src/games/game.h index 6e8f1352e..8b86d98ae 100644 --- a/src/games/game.h +++ b/src/games/game.h @@ -967,6 +967,8 @@ class GameRep : public std::enable_shared_from_this { virtual void BuildComputedValues() const {} }; +#include + class GameRep::Infosets { public: class iterator { @@ -993,10 +995,29 @@ class GameRep::Infosets { value_type operator*() const { + assert(inner_ != inner_end_); + assert(*inner_); + assert((*inner_)->IsValid()); + assert((*inner_)->m_game != nullptr); + assert((*inner_)->m_player != nullptr); + + auto g = (*inner_)->GetGame(); + assert(g); // i.e. g != nullptr return GameInfoset(*inner_); // GameObjectPtr constructor } - value_type operator->() const { return GameInfoset(*inner_); } + value_type operator->() const + { + assert(inner_ != inner_end_); + assert(*inner_); + assert((*inner_)->IsValid()); + assert((*inner_)->m_game != nullptr); + assert((*inner_)->m_player != nullptr); + + auto g = (*inner_)->GetGame(); + assert(g); // i.e. g != nullptrreturn + return GameInfoset(*inner_); + } iterator &operator++() { From 4d5949c5679ca7be8b447d341d7f742956335caf Mon Sep 17 00:00:00 2001 From: Theodore Turocy Date: Fri, 5 Dec 2025 17:36:40 +0000 Subject: [PATCH 11/19] Remember to turn on assertions for Python --- src/games/game.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/games/game.h b/src/games/game.h index 8b86d98ae..c08d41924 100644 --- a/src/games/game.h +++ b/src/games/game.h @@ -967,7 +967,8 @@ class GameRep : public std::enable_shared_from_this { virtual void BuildComputedValues() const {} }; -#include +#undef NDEBUG +#include class GameRep::Infosets { public: From d7856c2e3d57f072be4f24770ef45846681db340 Mon Sep 17 00:00:00 2001 From: Theodore Turocy Date: Fri, 5 Dec 2025 17:45:41 +0000 Subject: [PATCH 12/19] debugging sanity checks --- src/games/game.h | 44 ++++++++++++++++++++++++++++++++------------ 1 file changed, 32 insertions(+), 12 deletions(-) diff --git a/src/games/game.h b/src/games/game.h index c08d41924..55de70335 100644 --- a/src/games/game.h +++ b/src/games/game.h @@ -209,6 +209,7 @@ class GameInfosetRep : public std::enable_shared_from_this { } public: + void DebugSanityCheck() const; using Actions = ElementCollection; using Members = ElementCollection; @@ -382,6 +383,15 @@ class GamePlayerRep : public std::enable_shared_from_this { std::vector> m_strategies; public: + void DebugSanityCheck() const + { + if (!m_game) { + throw std::runtime_error("PlayerRep: m_game is null"); + } + if (!m_valid) { + throw std::runtime_error("PlayerRep: IsValid() is false"); + } + } using Infosets = ElementCollection; using Strategies = ElementCollection; @@ -970,6 +980,24 @@ class GameRep : public std::enable_shared_from_this { #undef NDEBUG #include +inline void GameInfosetRep::DebugSanityCheck() const +{ + if (!m_game) { + throw std::runtime_error("InfosetRep: m_game is null"); + } + if (!m_player) { + throw std::runtime_error("InfosetRep: m_player is null"); + } + if (!m_valid) { + throw std::runtime_error("InfosetRep: IsValid() is false"); + } + + // Optional deeper checks: + if (m_player->m_game != m_game) { + throw std::runtime_error("InfosetRep: player->m_game mismatch"); + } +} + class GameRep::Infosets { public: class iterator { @@ -996,12 +1024,8 @@ class GameRep::Infosets { value_type operator*() const { - assert(inner_ != inner_end_); - assert(*inner_); - assert((*inner_)->IsValid()); - assert((*inner_)->m_game != nullptr); - assert((*inner_)->m_player != nullptr); - + (*inner_)->DebugSanityCheck(); + (*inner_)->GetPlayer()->DebugSanityCheck(); auto g = (*inner_)->GetGame(); assert(g); // i.e. g != nullptr return GameInfoset(*inner_); // GameObjectPtr constructor @@ -1009,12 +1033,8 @@ class GameRep::Infosets { value_type operator->() const { - assert(inner_ != inner_end_); - assert(*inner_); - assert((*inner_)->IsValid()); - assert((*inner_)->m_game != nullptr); - assert((*inner_)->m_player != nullptr); - + (*inner_)->DebugSanityCheck(); + (*inner_)->GetPlayer()->DebugSanityCheck(); auto g = (*inner_)->GetGame(); assert(g); // i.e. g != nullptrreturn return GameInfoset(*inner_); From d5805cf3784785b7ddb01a55974d4d15834c6de9 Mon Sep 17 00:00:00 2001 From: Theodore Turocy Date: Sun, 7 Dec 2025 23:32:55 +0000 Subject: [PATCH 13/19] Try a more conservative implementation that simply wraps the public ranges for players and infosets. --- src/games/game.h | 125 +++++++++++++++-------------------------- src/games/gameobject.h | 2 +- src/games/gametree.h | 5 -- 3 files changed, 47 insertions(+), 85 deletions(-) diff --git a/src/games/game.h b/src/games/game.h index 55de70335..dc93e9940 100644 --- a/src/games/game.h +++ b/src/games/game.h @@ -999,114 +999,81 @@ inline void GameInfosetRep::DebugSanityCheck() const } class GameRep::Infosets { + Players m_players; + public: + explicit Infosets(const Players &outer) : m_players(outer) {} + class iterator { + using OuterIter = Players::iterator; + using InnerIter = GamePlayerRep::Infosets::iterator; + + OuterIter m_playerIterator, m_playerEnd; + InnerIter m_infosetIterator, m_infosetEnd; + + void next() + { + while (m_playerIterator != m_playerEnd && m_infosetIterator == m_infosetEnd) { + ++m_playerIterator; + if (m_playerIterator != m_playerEnd) { + auto infosets = (*m_playerIterator)->GetInfosets(); + m_infosetIterator = infosets.begin(); + m_infosetEnd = infosets.end(); + } + } + } + public: using iterator_category = std::forward_iterator_tag; using value_type = GameInfoset; + using reference = GameInfoset; + using pointer = GameInfoset; using difference_type = std::ptrdiff_t; - using pointer = value_type; - using reference = value_type; - - using outer_iter = std::vector>::const_iterator; - using inner_iter = std::vector>::const_iterator; iterator() = default; - iterator(std::shared_ptr game, outer_iter outer, outer_iter outer_end) - : game_(std::move(game)), outer_(outer), outer_end_(outer_end) + iterator(const OuterIter &p_playerIterator, const OuterIter &p_playerEnd) + : m_playerIterator(p_playerIterator), m_playerEnd(p_playerEnd) { - if (outer_ != outer_end_) { - init_inner(); - satisfy_invariant(); + if (m_playerIterator != m_playerEnd) { + const auto infosets = (*m_playerIterator)->GetInfosets(); + m_infosetIterator = infosets.begin(); + m_infosetEnd = infosets.end(); } + next(); } - value_type operator*() const - { - (*inner_)->DebugSanityCheck(); - (*inner_)->GetPlayer()->DebugSanityCheck(); - auto g = (*inner_)->GetGame(); - assert(g); // i.e. g != nullptr - return GameInfoset(*inner_); // GameObjectPtr constructor - } - - value_type operator->() const - { - (*inner_)->DebugSanityCheck(); - (*inner_)->GetPlayer()->DebugSanityCheck(); - auto g = (*inner_)->GetGame(); - assert(g); // i.e. g != nullptrreturn - return GameInfoset(*inner_); - } + reference operator*() const { return *m_infosetIterator; } + pointer operator->() const { return *m_infosetIterator; } iterator &operator++() { - ++inner_; - satisfy_invariant(); + ++m_infosetIterator; + next(); return *this; } - bool operator==(const iterator &other) const - { - // Compare end iterators correctly - if (outer_ == outer_end_ && other.outer_ == other.outer_end_) { - return true; - } - - return outer_ == other.outer_ && inner_ == other.inner_; - } - - bool operator!=(const iterator &other) const { return !(*this == other); } - - private: - bool is_end() const { return outer_ == outer_end_; } - - void init_inner() + iterator operator++(int) { - const auto &player = *outer_; - inner_ = player->m_infosets.begin(); - inner_end_ = player->m_infosets.end(); + iterator tmp = *this; + ++(*this); + return tmp; } - void satisfy_invariant() + friend bool operator==(const iterator &a, const iterator &b) { - while (!is_end() && inner_ == inner_end_) { - ++outer_; - if (!is_end()) { - init_inner(); - } - } + return a.m_playerIterator == b.m_playerIterator && + (a.m_playerIterator == a.m_playerEnd || a.m_infosetIterator == b.m_infosetIterator); } - // HOLD GAME ALIVE - std::shared_ptr game_; - - outer_iter outer_{}; - outer_iter outer_end_{}; - inner_iter inner_{}; - inner_iter inner_end_{}; + friend bool operator!=(const iterator &a, const iterator &b) { return !(a == b); } }; - // Construct a view storing a strong reference to the GameRep - explicit Infosets(std::shared_ptr game) - : game_(std::move(game)), players_(&game_->m_players) - { - } - - iterator begin() const { return iterator(game_, players_->begin(), players_->end()); } - - iterator end() const { return iterator(game_, players_->end(), players_->end()); } - -private: - std::shared_ptr game_; - const std::vector> *players_; + iterator begin() const { return {m_players.begin(), m_players.end()}; } + iterator end() const { return {m_players.end(), m_players.end()}; } }; -inline GameRep::Infosets GameRep::GetInfosets() const -{ - return Infosets(std::const_pointer_cast(this->shared_from_this())); -} +inline GameRep::Infosets GameRep::GetInfosets() const { return Infosets(GetPlayers()); } //======================================================================= // Inline members of game representation classes diff --git a/src/games/gameobject.h b/src/games/gameobject.h index c8c608a31..99d4920cc 100644 --- a/src/games/gameobject.h +++ b/src/games/gameobject.h @@ -135,7 +135,7 @@ template class GameObjectPtr { bool operator<(const GameObjectPtr &r) const { return (m_rep < r.m_rep); } operator bool() const noexcept { return m_rep != nullptr; } - operator std::shared_ptr() const { return m_rep; } + operator std::shared_ptr() const { return get_shared(); } }; template class ElementCollection { diff --git a/src/games/gametree.h b/src/games/gametree.h index cb88b9168..c8d4c0d6d 100644 --- a/src/games/gametree.h +++ b/src/games/gametree.h @@ -130,11 +130,6 @@ class GameTreeRep : public GameExplicitRep { //@{ /// Returns the iset'th information set in the game (numbered globally) GameInfoset GetInfoset(int iset) const override; - /// Returns the set of information sets in the game - Infosets GetInfosets() const override - { - return Infosets(std::const_pointer_cast(this->shared_from_this())); - } /// Sort the information sets for each player in a canonical order void SortInfosets() override; /// Returns the set of actions taken by the infoset's owner before reaching this infoset From 388f0c2af382dfe098250f8061986da5c92997b7 Mon Sep 17 00:00:00 2001 From: Theodore Turocy Date: Mon, 8 Dec 2025 06:15:23 +0000 Subject: [PATCH 14/19] Remove debugging assertions that cause ambiguous symbols problem. --- src/games/game.h | 31 ------------------------------- 1 file changed, 31 deletions(-) diff --git a/src/games/game.h b/src/games/game.h index dc93e9940..a8e1ae291 100644 --- a/src/games/game.h +++ b/src/games/game.h @@ -209,7 +209,6 @@ class GameInfosetRep : public std::enable_shared_from_this { } public: - void DebugSanityCheck() const; using Actions = ElementCollection; using Members = ElementCollection; @@ -383,15 +382,6 @@ class GamePlayerRep : public std::enable_shared_from_this { std::vector> m_strategies; public: - void DebugSanityCheck() const - { - if (!m_game) { - throw std::runtime_error("PlayerRep: m_game is null"); - } - if (!m_valid) { - throw std::runtime_error("PlayerRep: IsValid() is false"); - } - } using Infosets = ElementCollection; using Strategies = ElementCollection; @@ -977,27 +967,6 @@ class GameRep : public std::enable_shared_from_this { virtual void BuildComputedValues() const {} }; -#undef NDEBUG -#include - -inline void GameInfosetRep::DebugSanityCheck() const -{ - if (!m_game) { - throw std::runtime_error("InfosetRep: m_game is null"); - } - if (!m_player) { - throw std::runtime_error("InfosetRep: m_player is null"); - } - if (!m_valid) { - throw std::runtime_error("InfosetRep: IsValid() is false"); - } - - // Optional deeper checks: - if (m_player->m_game != m_game) { - throw std::runtime_error("InfosetRep: player->m_game mismatch"); - } -} - class GameRep::Infosets { Players m_players; From 1dabf1e6cb8decae8a366d8b2ba927de6e6feefd Mon Sep 17 00:00:00 2001 From: Theodore Turocy Date: Mon, 8 Dec 2025 06:33:43 +0000 Subject: [PATCH 15/19] Remove GameRep::Strategies based on dodgy iterator/range. --- src/games/game.h | 9 --- src/games/gameobject.h | 118 ------------------------------------ src/solvers/liap/nfgliap.cc | 48 ++++++++------- 3 files changed, 26 insertions(+), 149 deletions(-) diff --git a/src/games/game.h b/src/games/game.h index a8e1ae291..8a702800a 100644 --- a/src/games/game.h +++ b/src/games/game.h @@ -820,19 +820,10 @@ class GameRep : public std::enable_shared_from_this { /// @name Dimensions of the game //@{ - using Strategies = - NestedElementCollection<&GameRep::m_players, &GamePlayerRep::m_strategies, GameStrategy>; - /// The number of strategies for each player virtual Array NumStrategies() const = 0; /// Gets the i'th strategy in the game, numbered globally virtual GameStrategy GetStrategy(int p_index) const = 0; - /// Gets the collection of all strategies in the game - Strategies GetStrategies() const - { - BuildComputedValues(); - return Strategies(this); - } /// Creates a new strategy for the player virtual GameStrategy NewStrategy(const GamePlayer &p_player, const std::string &p_label) { diff --git a/src/games/gameobject.h b/src/games/gameobject.h index 99d4920cc..515ef1401 100644 --- a/src/games/gameobject.h +++ b/src/games/gameobject.h @@ -214,124 +214,6 @@ template class ElementCollection { iterator cend() const { return {m_owner, m_container, (m_owner) ? m_container->size() : 0}; } }; -template class NestedElementCollection { - template struct outer_element_of; - - template - struct outer_element_of> OuterClass::*> { - using outer_class = OuterClass; - using outer_elem = Elem; - using container = std::vector>; - }; - - template struct inner_element_of; - - template - struct inner_element_of> OuterElem::*> { - using outer_elem = OuterElem; - using inner_elem = InnerElem; - using container = std::vector>; - }; - - using OM = outer_element_of; - using IM = inner_element_of; - - using OuterClass = typename OM::outer_class; - using OuterElem = typename OM::outer_elem; - using InnerElem = typename IM::inner_elem; - -public: - class iterator { - using outer_iter = typename OM::container::const_iterator; - using inner_iter = typename IM::container::const_iterator; - - outer_iter m_outer, m_outerEnd; - inner_iter m_inner, m_innerEnd; - mutable ValueType m_cache; - - void advance() - { - while (m_outer != m_outerEnd && m_inner == m_innerEnd) { - ++m_outer; - if (m_outer != m_outerEnd) { - const auto &outerPtr = *m_outer; - const auto &innerVec = outerPtr.get()->*InnerMember; - m_inner = innerVec.begin(); - m_innerEnd = innerVec.end(); - } - } - } - - public: - using iterator_category = std::input_iterator_tag; - using value_type = ValueType; - using difference_type = std::ptrdiff_t; - using reference = ValueType; - using pointer = const ValueType *; - - iterator() = default; - - iterator(const OuterClass *p_obj, bool p_isEnd) - { - const auto &outerVec = p_obj->*OuterMember; - - m_outer = outerVec.begin(); - m_outerEnd = outerVec.end(); - - if (p_isEnd || m_outer == m_outerEnd) { - m_outer = m_outerEnd; - return; - } - - const auto &outerPtr = *m_outer; - const auto &innerVec = outerPtr.get()->*InnerMember; - - m_inner = innerVec.begin(); - m_innerEnd = innerVec.end(); - - advance(); - } - - reference operator*() const { return ValueType(*m_inner); } - - pointer operator->() const - { - m_cache = ValueType(*m_inner); - return &m_cache; - } - - iterator &operator++() - { - ++m_inner; - advance(); - return *this; - } - - iterator operator++(int) - { - iterator tmp = *this; - ++(*this); - return tmp; - } - - bool operator==(const iterator &p_other) const - { - return m_outer == p_other.m_outer && (m_outer == m_outerEnd || m_inner == p_other.m_inner); - } - - bool operator!=(const iterator &p_other) const { return !(*this == p_other); } - }; - -private: - const OuterClass *m_obj; - -public: - explicit NestedElementCollection(const OuterClass *p_obj) : m_obj(p_obj) {} - - iterator begin() const { return {m_obj, false}; } - iterator end() const { return {m_obj, true}; } -}; - } // end namespace Gambit #endif // GAMBIT_GAMES_GAMEOBJECT_H diff --git a/src/solvers/liap/nfgliap.cc b/src/solvers/liap/nfgliap.cc index ee65a1a05..f1fe4a11a 100644 --- a/src/solvers/liap/nfgliap.cc +++ b/src/solvers/liap/nfgliap.cc @@ -63,32 +63,34 @@ class StrategicLyapunovFunction : public FunctionOnSimplices { inline double sum_player_probs(const MixedStrategyProfile &p_profile, const GamePlayer &p_player) { - return sum_function(p_player->GetStrategies(), - [&p_profile](const auto &s) -> double { return p_profile[s]; }); + auto strategies = p_player->GetStrategies(); + return std::accumulate( + strategies.cbegin(), strategies.cend(), 0.0, + [p_profile](double t, const GameStrategy &s) { return t + p_profile[s]; }); } double StrategicLyapunovFunction::Value(const Vector &v) const { m_profile = v; double value = 0; - // Liapunov function proper - for (const auto &player : m_profile.GetGame()->GetPlayers()) { - const auto payoff = m_profile.GetPayoff(player); - for (const auto strategy : player->GetStrategies()) { - value += sqr(std::max(m_scale * (m_profile.GetPayoff(strategy) - payoff), 0.0)); + // Liapunov function proper - should be replaced with call to profile once + // the penalty is removed from that implementation. + for (auto player : m_profile.GetGame()->GetPlayers()) { + for (auto strategy : player->GetStrategies()) { + value += sqr( + std::max(m_scale * (m_profile.GetPayoff(strategy) - m_profile.GetPayoff(player)), 0.0)); } } // Penalty function for non-negativity constraint for each strategy - value += m_penalty * - sum_function(m_profile.GetGame()->GetStrategies(), [this](const auto &s) -> double { - return sqr(std::min(m_profile[s], 0.0)); - }); - + for (auto player : m_profile.GetGame()->GetPlayers()) { + for (auto strategy : player->GetStrategies()) { + value += m_penalty * sqr(std::min(m_profile[strategy], 0.0)); + } + } // Penalty function for sum-to-one constraint for each player - value += - m_penalty * sum_function(m_profile.GetGame()->GetPlayers(), [this](const auto &p) -> double { - return sqr(sum_player_probs(m_profile, p) - 1.0); - }); + for (auto player : m_profile.GetGame()->GetPlayers()) { + value += m_penalty * sqr(sum_player_probs(m_profile, player) - 1.0); + } return value; } @@ -96,10 +98,10 @@ double StrategicLyapunovFunction::LiapDerivValue(const MixedStrategyProfileGetPlayers()) { - const auto payoff = p_profile.GetPayoff(player); - for (const auto &strategy : player->GetStrategies()) { - const double loss = sqr(m_scale) * (p_profile.GetPayoff(strategy) - payoff); + for (auto player : m_game->GetPlayers()) { + for (auto strategy : player->GetStrategies()) { + const double loss = + sqr(m_scale) * (p_profile.GetPayoff(strategy) - p_profile.GetPayoff(player)); if (loss <= 0.0) { continue; } @@ -135,8 +137,10 @@ namespace { MixedStrategyProfile EnforceNonnegativity(const MixedStrategyProfile &p_profile) { auto profile = p_profile; - for (const auto &strategy : p_profile.GetGame()->GetStrategies()) { - profile[strategy] = std::max(profile[strategy], 0.0); + for (auto player : p_profile.GetGame()->GetPlayers()) { + for (auto strategy : player->GetStrategies()) { + profile[strategy] = std::max(profile[strategy], 0.0); + } } return profile.Normalize(); } From 07b9ee74b3d1772673c18fbb277431584f2f2c64 Mon Sep 17 00:00:00 2001 From: Theodore Turocy Date: Mon, 8 Dec 2025 06:53:35 +0000 Subject: [PATCH 16/19] Revert unrelated changes. --- .github/workflows/python.yml | 2 +- src/games/gameobject.h | 2 +- src/games/gametable.cc | 16 +--------------- src/games/gametable.h | 6 +----- src/games/gametree.cc | 26 ++++---------------------- src/games/gametree.h | 6 +----- 6 files changed, 9 insertions(+), 49 deletions(-) diff --git a/.github/workflows/python.yml b/.github/workflows/python.yml index d0eb8f878..794340dde 100644 --- a/.github/workflows/python.yml +++ b/.github/workflows/python.yml @@ -107,4 +107,4 @@ jobs: run: | python -m pip install -v . - name: Run tests - run: pytest -vv + run: pytest diff --git a/src/games/gameobject.h b/src/games/gameobject.h index 515ef1401..a8328fe3c 100644 --- a/src/games/gameobject.h +++ b/src/games/gameobject.h @@ -135,7 +135,7 @@ template class GameObjectPtr { bool operator<(const GameObjectPtr &r) const { return (m_rep < r.m_rep); } operator bool() const noexcept { return m_rep != nullptr; } - operator std::shared_ptr() const { return get_shared(); } + operator std::shared_ptr() const { return m_rep; } }; template class ElementCollection { diff --git a/src/games/gametable.cc b/src/games/gametable.cc index 66bdfcbf9..252e5790e 100644 --- a/src/games/gametable.cc +++ b/src/games/gametable.cc @@ -21,7 +21,6 @@ // #include -#include #include "gambit.h" #include "gametable.h" @@ -67,7 +66,7 @@ std::shared_ptr TablePureStrategyProfileRep::Copy() cons Game NewTable(const std::vector &p_dim, bool p_sparseOutcomes /*= false*/) { - return GameTableRep::Create(p_dim, p_sparseOutcomes); + return std::make_shared(p_dim, p_sparseOutcomes); } //------------------------------------------------------------------------ @@ -264,10 +263,6 @@ template class TableMixedStrategyProfileRep; GameTableRep::GameTableRep(const std::vector &dim, bool p_sparseOutcomes /* = false */) : m_results(std::accumulate(dim.begin(), dim.end(), 1, std::multiplies<>())) -{ -} - -void GameTableRep::Initialize(const std::vector &dim, bool p_sparseOutcomes /* = false */) { for (const auto &nstrat : dim) { m_players.push_back(std::make_shared(this, m_players.size() + 1, nstrat)); @@ -292,15 +287,6 @@ void GameTableRep::Initialize(const std::vector &dim, bool p_sparseOutcomes } } -std::shared_ptr GameTableRep::Create(const std::vector &dim, - bool sparseOutcomes) -{ - auto rep = std::shared_ptr(new GameTableRep(dim, sparseOutcomes)); - rep->Initialize(dim, sparseOutcomes); - - return rep; -} - Game GameTableRep::Copy() const { std::ostringstream os; diff --git a/src/games/gametable.h b/src/games/gametable.h index 193c58173..3aab1dd38 100644 --- a/src/games/gametable.h +++ b/src/games/gametable.h @@ -44,16 +44,12 @@ class GameTableRep : public GameExplicitRep { void RebuildTable(); //@} - explicit GameTableRep(const std::vector &p_dim, bool p_sparseOutcomes = false); - void Initialize(const std::vector &dim, bool p_sparseOutcomes); - public: /// @name Lifecycle //@{ /// Construct a new table game with the given dimension /// If p_sparseOutcomes = true, outcomes for all contingencies are left null - static std::shared_ptr Create(const std::vector &dim, - bool sparseOutcomes = false); + explicit GameTableRep(const std::vector &p_dim, bool p_sparseOutcomes = false); Game Copy() const override; //@} diff --git a/src/games/gametree.cc b/src/games/gametree.cc index 0c781be07..33509228e 100644 --- a/src/games/gametree.cc +++ b/src/games/gametree.cc @@ -701,28 +701,10 @@ GameInfoset GameTreeRep::InsertMove(GameNode p_node, GameInfoset p_infoset) // GameTreeRep: Lifecycle //------------------------------------------------------------------------ -GameTreeRep::GameTreeRep() = default; - -void GameTreeRep::Initialize() -{ - auto self = shared_from_this(); - - // Now construct children using the VALID GameTreeRep* - m_root = std::make_shared(self.get(), nullptr); - m_chance = std::make_shared(self.get(), 0); - - // If GameNodeRep creates children of its own, same pattern applies -} - -std::shared_ptr GameTreeRep::Create() +GameTreeRep::GameTreeRep() + : m_root(std::make_shared(this, nullptr)), + m_chance(std::make_shared(this, 0)) { - // Construct object under shared_ptr CONTROL BLOCK - auto rep = std::shared_ptr(new GameTreeRep()); - - // Now safe to construct children because shared_from_this works - rep->Initialize(); - - return rep; } GameTreeRep::~GameTreeRep() @@ -739,7 +721,7 @@ Game GameTreeRep::Copy() const return ReadGame(is); } -Game NewTree() { return GameTreeRep::Create(); } +Game NewTree() { return std::make_shared(); } //------------------------------------------------------------------------ // GameTreeRep: General data access diff --git a/src/games/gametree.h b/src/games/gametree.h index c8d4c0d6d..e77500964 100644 --- a/src/games/gametree.h +++ b/src/games/gametree.h @@ -68,14 +68,10 @@ class GameTreeRep : public GameExplicitRep { void CopySubtree(GameNodeRep *, GameNodeRep *, GameNodeRep *); //@} - GameTreeRep(); - void Initialize(); - public: /// @name Lifecycle //@{ - - static std::shared_ptr Create(); + GameTreeRep(); ~GameTreeRep() override; Game Copy() const override; //@} From b4774c2be34b7e05fab5b4138d229d97a71095be Mon Sep 17 00:00:00 2001 From: Theodore Turocy Date: Mon, 8 Dec 2025 08:48:03 +0000 Subject: [PATCH 17/19] Use new GetInfosets() in liap --- src/core/array.h | 1 + src/solvers/liap/efgliap.cc | 43 ++++++++++++------------------------- 2 files changed, 15 insertions(+), 29 deletions(-) diff --git a/src/core/array.h b/src/core/array.h index f2f44af77..a7e2e294a 100644 --- a/src/core/array.h +++ b/src/core/array.h @@ -42,6 +42,7 @@ template class Array { public: using iterator = typename std::vector::iterator; using const_iterator = typename std::vector::const_iterator; + using value_type = typename std::vector::value_type; using reference = typename std::vector::reference; using const_reference = typename std::vector::const_reference; diff --git a/src/solvers/liap/efgliap.cc b/src/solvers/liap/efgliap.cc index 2add29501..7d55749d6 100644 --- a/src/solvers/liap/efgliap.cc +++ b/src/solvers/liap/efgliap.cc @@ -38,18 +38,10 @@ class AgentLyapunovFunction : public FunctionOnSimplices { : m_game(p_start.GetGame()), m_profile(p_start) { m_scale = m_game->GetMaxPayoff() - m_game->GetMinPayoff(); - if (m_scale == 0.0) { - m_scale = 1.0; - } - else { - m_scale = 1.0 / m_scale; - } - - for (const auto &player : m_game->GetPlayers()) { - for (const auto &infoset : player->GetInfosets()) { - m_shape.push_back(infoset->GetActions().size()); - } - } + m_scale = (m_scale == 0.0) ? 1.0 : 1.0 / m_scale; + std::transform( + m_game->GetInfosets().begin(), m_game->GetInfosets().end(), std::back_inserter(m_shape), + [](const auto &infoset) -> std::size_t { return infoset->GetActions().size(); }); } ~AgentLyapunovFunction() override = default; @@ -81,14 +73,11 @@ double AgentLyapunovFunction::PenalizedLiapValue(const MixedBehaviorProfile &p_profile) const { double value = 0.0; - // Liapunov function proper - should be replaced with call to profile once - // the penalty is removed from that implementation. - for (auto player : p_profile.GetGame()->GetPlayers()) { - for (auto infoset : player->GetInfosets()) { - for (auto action : infoset->GetActions()) { - value += sqr( - std::max(m_scale * (p_profile.GetPayoff(action) - p_profile.GetPayoff(infoset)), 0.0)); - } + // Liapunov function proper. + for (const auto &infoset : p_profile.GetGame()->GetInfosets()) { + for (const auto &action : infoset->GetActions()) { + value += sqr( + std::max(m_scale * (p_profile.GetPayoff(action) - p_profile.GetPayoff(infoset)), 0.0)); } } // Penalty function for non-negativity constraint for each action @@ -96,10 +85,8 @@ AgentLyapunovFunction::PenalizedLiapValue(const MixedBehaviorProfile &p_ value += m_penalty * sqr(std::min(element, 0.0)); } // Penalty function for sum-to-one constraint for each action - for (auto player : p_profile.GetGame()->GetPlayers()) { - for (auto infoset : player->GetInfosets()) { - value += m_penalty * sqr(sum_infoset_probs(m_profile, infoset) - 1.0); - } + for (const auto &infoset : p_profile.GetGame()->GetInfosets()) { + value += m_penalty * sqr(sum_infoset_probs(m_profile, infoset) - 1.0); } return value; } @@ -131,11 +118,9 @@ namespace { MixedBehaviorProfile EnforceNonnegativity(const MixedBehaviorProfile &p_profile) { auto profile = p_profile; - for (auto player : p_profile.GetGame()->GetPlayers()) { - for (auto infoset : player->GetInfosets()) { - for (auto action : infoset->GetActions()) { - profile[action] = std::max(profile[action], 0.0); - } + for (const auto &infoset : p_profile.GetGame()->GetInfosets()) { + for (const auto &action : infoset->GetActions()) { + profile[action] = std::max(profile[action], 0.0); } } return profile.Normalize(); From e3533ad3b769021b0d3c2a889738192aeb497e9a Mon Sep 17 00:00:00 2001 From: Theodore Turocy Date: Mon, 8 Dec 2025 08:55:34 +0000 Subject: [PATCH 18/19] use sum_function in liap --- src/solvers/liap/efgliap.cc | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/solvers/liap/efgliap.cc b/src/solvers/liap/efgliap.cc index 7d55749d6..d5a148057 100644 --- a/src/solvers/liap/efgliap.cc +++ b/src/solvers/liap/efgliap.cc @@ -75,10 +75,10 @@ AgentLyapunovFunction::PenalizedLiapValue(const MixedBehaviorProfile &p_ double value = 0.0; // Liapunov function proper. for (const auto &infoset : p_profile.GetGame()->GetInfosets()) { - for (const auto &action : infoset->GetActions()) { - value += sqr( - std::max(m_scale * (p_profile.GetPayoff(action) - p_profile.GetPayoff(infoset)), 0.0)); - } + double infosetValue = p_profile.GetPayoff(infoset); + value += sum_function(infoset->GetActions(), [&](const auto &action) { + return sqr(std::max(m_scale * (p_profile.GetPayoff(action) - infosetValue), 0.0)); + }); } // Penalty function for non-negativity constraint for each action for (auto element : static_cast &>(p_profile)) { From bd870f63493b3c327f2790999763f8f5f889ee2d Mon Sep 17 00:00:00 2001 From: Theodore Turocy Date: Mon, 8 Dec 2025 09:05:12 +0000 Subject: [PATCH 19/19] Some final low-hanging fruit in efgliap --- src/solvers/liap/efgliap.cc | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/src/solvers/liap/efgliap.cc b/src/solvers/liap/efgliap.cc index d5a148057..4eb888a33 100644 --- a/src/solvers/liap/efgliap.cc +++ b/src/solvers/liap/efgliap.cc @@ -64,9 +64,8 @@ class AgentLyapunovFunction : public FunctionOnSimplices { inline double sum_infoset_probs(const MixedBehaviorProfile &p_profile, const GameInfoset &p_infoset) { - auto actions = p_infoset->GetActions(); - return std::accumulate(actions.begin(), actions.end(), 0.0, - [p_profile](double t, const GameAction &a) { return t + p_profile[a]; }); + return sum_function(p_infoset->GetActions(), + [&p_profile](const auto &action) { return p_profile[action]; }); } double @@ -76,7 +75,7 @@ AgentLyapunovFunction::PenalizedLiapValue(const MixedBehaviorProfile &p_ // Liapunov function proper. for (const auto &infoset : p_profile.GetGame()->GetInfosets()) { double infosetValue = p_profile.GetPayoff(infoset); - value += sum_function(infoset->GetActions(), [&](const auto &action) { + value += sum_function(infoset->GetActions(), [&](const auto &action) -> double { return sqr(std::max(m_scale * (p_profile.GetPayoff(action) - infosetValue), 0.0)); }); } @@ -85,9 +84,9 @@ AgentLyapunovFunction::PenalizedLiapValue(const MixedBehaviorProfile &p_ value += m_penalty * sqr(std::min(element, 0.0)); } // Penalty function for sum-to-one constraint for each action - for (const auto &infoset : p_profile.GetGame()->GetInfosets()) { - value += m_penalty * sqr(sum_infoset_probs(m_profile, infoset) - 1.0); - } + value += sum_function(p_profile.GetGame()->GetInfosets(), [&](const auto &infoset) -> double { + return m_penalty * sqr(sum_infoset_probs(m_profile, infoset) - 1.0); + }); return value; }