From 02d57ed812ae2aa5823e1c63aedb0cf8fe715bd7 Mon Sep 17 00:00:00 2001 From: Theodore Turocy Date: Tue, 29 Sep 2026 06:36:17 +0100 Subject: [PATCH 1/2] Index outcome labels to make outcome creation and relabelling constant time. --- src/games/game.cc | 20 +++++-------- src/games/game.h | 52 +++++++++++++++++++++++++------- src/games/gametable.cc | 3 +- src/games/gametree.cc | 2 +- tests/test_outcome_mutations.py | 53 +++++++++++++++++++++++++++++++++ 5 files changed, 106 insertions(+), 24 deletions(-) diff --git a/src/games/game.cc b/src/games/game.cc index e1ef620651..b6c7dfdd23 100644 --- a/src/games/game.cc +++ b/src/games/game.cc @@ -304,20 +304,12 @@ void GameRep::RelabelOutcomes(const std::map &p_labels std::map assignment; std::set relabeled; for (const auto &[old_label, new_label] : p_labels) { - GameOutcomeRep *match = nullptr; - for (const auto &outcome : m_outcomes) { - if (outcome->GetLabel() == old_label) { - if (match) { - throw ValueException("Outcome label '" + old_label + "' is ambiguous in this game"); - } - match = outcome.get(); - } - } - if (!match) { + const auto entry = m_outcomeLabels.find(old_label); + if (entry == m_outcomeLabels.end()) { throw ValueException("No outcome with label '" + old_label + "' in this game"); } - assignment[match] = new_label; - relabeled.insert(match); + assignment[entry->second] = new_label; + relabeled.insert(entry->second); } // Replacement labels must be legal, unique against untouched outcomes, and pairwise distinct std::set targets; @@ -328,8 +320,12 @@ void GameRep::RelabelOutcomes(const std::map &p_labels "' would be duplicated by the relabelling"); } } + for (const auto &[outcome, new_label] : assignment) { + UnindexOutcomeLabel(outcome); + } for (const auto &[outcome, new_label] : assignment) { outcome->m_label = new_label; + m_outcomeLabels[new_label] = outcome; } } diff --git a/src/games/game.h b/src/games/game.h index 08c11b5a03..39629fed63 100644 --- a/src/games/game.h +++ b/src/games/game.h @@ -31,6 +31,7 @@ #include #include #include +#include #include "core/array.h" #include "number.h" @@ -590,6 +591,8 @@ class GameRep : public std::enable_shared_from_this { protected: std::vector> m_players; std::vector> m_outcomes; + /// The outcomes of the game indexed by label; outcome labels are unique within a game + std::unordered_map m_outcomeLabels; /// The game's null outcome, shared by every node or contingency with no outcome attached. std::shared_ptr m_nullOutcome; mutable CartesianProductSpace m_pureStrategies; @@ -612,6 +615,12 @@ class GameRep : public std::enable_shared_from_this { const std::set &p_ignore = {}) const; /// Invalidate and remove the given outcomes from the game, renumbering the survivors. void EraseOutcomes(const std::set &p_outcomes); + /// Append a newly created outcome to the outcomes of the game. + void AddOutcome(const std::shared_ptr &p_outcome); + /// Rebuild the index of outcome labels from the outcomes of the game. + void IndexOutcomeLabels(); + /// Remove the entry for p_outcome's label from the index, if it refers to p_outcome. + void UnindexOutcomeLabel(const GameOutcomeRep *p_outcome); //@} /// Hooks for derived classes to update lazily-computed orderings if required @@ -1018,8 +1027,10 @@ inline void GameOutcomeRep::SetLabel(const std::string &p_label) if (p_label == m_label) { return; } - GetGame()->CheckOutcomeLabel(p_label); + m_game->CheckOutcomeLabel(p_label); + m_game->UnindexOutcomeLabel(this); m_label = p_label; + m_game->m_outcomeLabels[m_label] = this; } template const T &GameOutcomeRep::GetPayoff(const GamePlayer &p_player) const @@ -1087,20 +1098,41 @@ inline void GameRep::CheckOutcomeLabel(const std::string &p_label, throw ValueException("Outcome label must not be empty"); } CheckLabel(p_label); + if (const auto entry = m_outcomeLabels.find(p_label); + entry != m_outcomeLabels.end() && !p_ignore.contains(entry->second)) { + throw ValueException("Outcome label must be unique within the game"); + } +} +inline void GameRep::AddOutcome(const std::shared_ptr &p_outcome) +{ + m_outcomes.push_back(p_outcome); + // An outcome being absorbed may still hold this label; the new outcome takes it over + m_outcomeLabels[p_outcome->m_label] = p_outcome.get(); +} +inline void GameRep::IndexOutcomeLabels() +{ + m_outcomeLabels.clear(); for (const auto &outcome : m_outcomes) { - if (outcome->GetLabel() == p_label && !p_ignore.contains(outcome.get())) { - throw ValueException("Outcome label must be unique within the game"); - } + m_outcomeLabels[outcome->m_label] = outcome.get(); } } -inline void GameRep::EraseOutcomes(const std::set &p_outcomes) +inline void GameRep::UnindexOutcomeLabel(const GameOutcomeRep *p_outcome) { - for (const auto *outcome : p_outcomes) { - auto member = std::find_if(m_outcomes.begin(), m_outcomes.end(), - [outcome](const auto &c) { return c.get() == outcome; }); - (*member)->Invalidate(); - m_outcomes.erase(member); + if (const auto entry = m_outcomeLabels.find(p_outcome->m_label); + entry != m_outcomeLabels.end() && entry->second == p_outcome) { + m_outcomeLabels.erase(entry); } +} +inline void GameRep::EraseOutcomes(const std::set &p_outcomes) +{ + std::erase_if(m_outcomes, [&](const std::shared_ptr &c) { + if (!p_outcomes.contains(c.get())) { + return false; + } + UnindexOutcomeLabel(c.get()); + c->Invalidate(); + return true; + }); std::for_each( m_outcomes.begin(), m_outcomes.end(), [outc = 1](const std::shared_ptr &c) mutable { c->m_number = outc++; }); diff --git a/src/games/gametable.cc b/src/games/gametable.cc index 07a078a730..9bbc141189 100644 --- a/src/games/gametable.cc +++ b/src/games/gametable.cc @@ -373,6 +373,7 @@ GameTableRep::GameTableRep(const std::vector &dim, bool p_sparseOutcomes /* }); std::transform(m_outcomes.begin(), m_outcomes.end(), m_results.begin(), [](const std::shared_ptr &c) { return c.get(); }); + IndexOutcomeLabels(); } } @@ -539,7 +540,7 @@ GameTableRep::MakeOutcome(const std::vector> &p_contin IncrementVersion(); auto outcome = std::make_shared(this, m_outcomes.size() + 1, p_label); - m_outcomes.push_back(outcome); + AddOutcome(outcome); for (const auto &[pl, player] : enumerate(m_players)) { outcome->SetPayoff(player, p_payoffs[pl]); } diff --git a/src/games/gametree.cc b/src/games/gametree.cc index 06f7f30e68..e5a37a1b0f 100644 --- a/src/games/gametree.cc +++ b/src/games/gametree.cc @@ -1880,7 +1880,7 @@ GameOutcome GameTreeRep::MakeOutcome(const std::vector &p_nodes, IncrementVersion(); auto outcome = std::make_shared(this, m_outcomes.size() + 1, p_label); - m_outcomes.push_back(outcome); + AddOutcome(outcome); for (const auto &[pl, player] : enumerate(m_players)) { outcome->SetPayoff(player, p_payoffs[pl]); } diff --git a/tests/test_outcome_mutations.py b/tests/test_outcome_mutations.py index 2444d5ce08..43b5f190b7 100644 --- a/tests/test_outcome_mutations.py +++ b/tests/test_outcome_mutations.py @@ -198,3 +198,56 @@ def test_outcome_relabel_duplicate_rejected_and_label_unchanged(): with pytest.raises(ValueError): game.relabel_outcomes({"lose": "win"}) assert set(game.get_outcomes()) == {"win", "lose"} + + +def test_outcome_relabel_swap_keeps_labels_consistent(): + """Swapping two outcomes' labels in one relabelling leaves each label naming the + other outcome, with both labels still in use.""" + game = gbt.ExtensiveGame(players=["A", "B"]) + game.append_move(gbt.H.path(), "A", ["x", "y"]) + game.make_outcome(gbt.H.path("x"), {"A": 1, "B": 0}, "first") + game.make_outcome(gbt.H.path("y"), {"A": 2, "B": 0}, "second") + game.relabel_outcomes({"first": "second", "second": "first"}) + assert game.get_outcome(gbt.H.path("x")) == "second" + assert game.get_outcome(gbt.H.path("y")) == "first" + assert game.get_outcome_payoffs("second")["A"] == 1 + assert game.get_outcome_payoffs("first")["A"] == 2 + with pytest.raises(ValueError): + game.relabel_outcomes({"first": "second"}) + + +def test_outcome_label_freed_by_relabel_can_be_reused(): + game = gbt.ExtensiveGame(players=["A"]) + game.append_move(gbt.H.path(), "A", ["x", "y"]) + game.make_outcome(gbt.H.path("x"), {"A": 1}, "old") + game.relabel_outcomes({"old": "new"}) + game.make_outcome(gbt.H.path("y"), {"A": 2}, "old") + assert set(game.get_outcomes()) == {"old", "new"} + with pytest.raises(ValueError): + game.relabel_outcomes({"old": "new"}) + + +def test_outcome_label_reused_on_absorption_refers_to_new_outcome(): + """A label taken over from an absorbed outcome names the new outcome, and can then be + relabelled and freed like any other.""" + game = gbt.ExtensiveGame(["Alice"]) + game.append_move(gbt.H.path(), "Alice", ["U", "D"]) + game.make_outcome(gbt.H.path("U"), {"Alice": 1}, "w") + game.make_outcome(gbt.H.path(...), {"Alice": 2}, "w") + game.relabel_outcomes({"w": "v"}) + assert game.get_outcomes() == ["v"] + assert game.get_outcome_payoffs("v")["Alice"] == 2 + game.make_outcome(gbt.H.path(...), {"Alice": 3}, "w") + assert game.get_outcomes() == ["w"] + + +def test_outcome_labels_of_dense_strategic_game_are_indexed(): + """Outcomes created in bulk with a strategic game have unique labels which can be + relabelled, and a duplicate is refused.""" + game = gbt.StrategicGame.from_arrays([[1, 2], [3, 4]], [[4, 3], [2, 1]]) + labels = game.get_outcomes() + assert len(set(labels)) == len(labels) == 4 + game.relabel_outcomes({labels[0]: "renamed"}) + assert "renamed" in game.get_outcomes() + with pytest.raises(ValueError): + game.relabel_outcomes({labels[1]: "renamed"}) From 633ce011400933eb987c558e89b9c1ae29787423 Mon Sep 17 00:00:00 2001 From: Theodore Turocy Date: Tue, 29 Sep 2026 07:06:42 +0100 Subject: [PATCH 2/2] Reduce the cost of navigating the game tree through node accessors. GameNodeRep::GetPriorAction scanned the actions at its parent's information set, calling GetChild on each, to discover which child the node was; GetNextSibling and GetPriorSibling searched the parent's children. Each node now records its position among its parent's children when the tree is numbered, so these are constant time. The position is recorded rather than the action because a node's children keep their positions when it moves to another information set, which does not renumber the tree; every change to a node's children does. GetChild now checks the action's information set by raw pointer, and GameObjectPtr::operator-> returns a raw pointer after its null and validity checks, rather than a shared pointer by value: the handle already keeps the element alive, so each member access no longer pays for reference counting. --- src/games/gameobject.h | 6 ++++-- src/games/gametree.cc | 20 ++++++++++---------- src/games/gametree.h | 7 +++++-- 3 files changed, 19 insertions(+), 14 deletions(-) diff --git a/src/games/gameobject.h b/src/games/gameobject.h index 47200aaa8c..fe650a4ef3 100644 --- a/src/games/gameobject.h +++ b/src/games/gameobject.h @@ -80,15 +80,17 @@ template class GameObjectPtr { /// /// @exception NullException if the object holds a reference to a null element /// @exception InvalidObjectException if the element referred to has been deleted from its game - std::shared_ptr operator->() const + T *operator->() const { + // The handle itself keeps the element alive, so there is no need to return a + // shared pointer (and pay for its reference counting) on every member access. if (!m_rep) { throw NullException(); } if (!m_rep->IsValid()) { throw InvalidObjectException(); } - return m_rep; + return m_rep.get(); } /// Access the shared pointer to the object representing the game element diff --git a/src/games/gametree.cc b/src/games/gametree.cc index e5a37a1b0f..639125f9ca 100644 --- a/src/games/gametree.cc +++ b/src/games/gametree.cc @@ -376,8 +376,8 @@ GameNode GameNodeRep::GetNextSibling() const if (!m_parent || m_parent->m_children.back().get() == this) { return nullptr; } - return *std::next( - std::find(m_parent->m_children.begin(), m_parent->m_children.end(), shared_from_this())); + m_game->EnsureNodeOrdering(); + return m_parent->m_children[m_childIndex + 1]; } GameNode GameNodeRep::GetPriorSibling() const @@ -385,8 +385,8 @@ GameNode GameNodeRep::GetPriorSibling() const if (!m_parent || m_parent->m_children.front().get() == this) { return nullptr; } - return *std::prev( - std::find(m_parent->m_children.begin(), m_parent->m_children.end(), shared_from_this())); + m_game->EnsureNodeOrdering(); + return m_parent->m_children[m_childIndex - 1]; } GameAction GameNodeRep::GetPriorAction() const @@ -394,12 +394,9 @@ GameAction GameNodeRep::GetPriorAction() const if (!m_parent) { return nullptr; } - for (const auto &action : m_parent->m_infoset->m_actions) { - if (m_parent->GetChild(action).get() == this) { - return action; - } - } - return nullptr; + // Children are in the order of the actions at their parent's information set + m_game->EnsureNodeOrdering(); + return m_parent->m_infoset->m_actions[m_childIndex]; } GameAction GameNodeRep::GetOwnPriorAction() const @@ -1135,6 +1132,9 @@ void GameTreeRep::EnsureNodeOrdering() const int nodeindex = 1; for (const auto &node : GetNodes()) { node->m_number = nodeindex++; + for (std::size_t i = 0; i < node->m_children.size(); i++) { + node->m_children[i]->m_childIndex = i; + } } }); } diff --git a/src/games/gametree.h b/src/games/gametree.h index 1f72762063..89f8256389 100644 --- a/src/games/gametree.h +++ b/src/games/gametree.h @@ -40,6 +40,7 @@ namespace Gambit { class GameActionRep : public std::enable_shared_from_this { friend class GameTreeRep; friend class GameInfosetRep; + friend class GameNodeRep; template friend class MixedBehaviorProfile; bool m_valid{true}; @@ -192,6 +193,8 @@ class GameNodeRep : public std::enable_shared_from_this { bool m_valid{true}; int m_number{0}; + /// Position of the node among its parent's children, set with the node ordering + std::size_t m_childIndex{0}; GameRep *m_game; std::string m_label; GameInfosetRep *m_infoset{nullptr}; @@ -233,10 +236,10 @@ class GameNodeRep : public std::enable_shared_from_this { int GetNumber() const; GameNode GetChild(const GameAction &p_action) { - if (p_action->GetInfoset().get() != m_infoset) { + if (p_action->m_infoset != m_infoset) { throw MismatchException("Action is from a different information set than node"); } - return m_children.at(p_action->GetNumber() - 1); + return m_children.at(p_action->m_number - 1); } Children GetChildren() const {