diff --git a/src/games/game.cc b/src/games/game.cc index e1ef62065..b6c7dfdd2 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 08c11b5a0..39629fed6 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/gameobject.h b/src/games/gameobject.h index 47200aaa8..fe650a4ef 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/gametable.cc b/src/games/gametable.cc index 07a078a73..9bbc14118 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 06f7f30e6..639125f9c 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; + } } }); } @@ -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/src/games/gametree.h b/src/games/gametree.h index 1f7276206..89f825638 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 { diff --git a/tests/test_outcome_mutations.py b/tests/test_outcome_mutations.py index 2444d5ce0..43b5f190b 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"})