Improvements to building and accessing trees

src/games/game.cc

@@ -304,20 +304,12 @@ void GameRep::RelabelOutcomes(const std::map<std::string, std::string> &p_labels
   std::map<GameOutcomeRep *, std::string> assignment;
   std::set<const GameOutcomeRep *> 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<std::string> targets;
@@ -328,8 +320,12 @@ void GameRep::RelabelOutcomes(const std::map<std::string, std::string> &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;
   }
 }
 

src/games/game.h

@@ -31,6 +31,7 @@
 #include <random>
 #include <set>
 #include <stack>
+#include <unordered_map>
 
 #include "core/array.h"
 #include "number.h"
@@ -590,6 +591,8 @@ class GameRep : public std::enable_shared_from_this<GameRep> {
 protected:
   std::vector<std::shared_ptr<GamePlayerRep>> m_players;
   std::vector<std::shared_ptr<GameOutcomeRep>> m_outcomes;
+  /// The outcomes of the game indexed by label; outcome labels are unique within a game
+  std::unordered_map<std::string, GameOutcomeRep *> m_outcomeLabels;
   /// The game's null outcome, shared by every node or contingency with no outcome attached.
   std::shared_ptr<GameOutcomeRep> m_nullOutcome;
   mutable CartesianProductSpace m_pureStrategies;
@@ -612,6 +615,12 @@ class GameRep : public std::enable_shared_from_this<GameRep> {
                          const std::set<const GameOutcomeRep *> &p_ignore = {}) const;
   /// Invalidate and remove the given outcomes from the game, renumbering the survivors.
   void EraseOutcomes(const std::set<const GameOutcomeRep *> &p_outcomes);
+  /// Append a newly created outcome to the outcomes of the game.
+  void AddOutcome(const std::shared_ptr<GameOutcomeRep> &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 <class T> 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<GameOutcomeRep> &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<const GameOutcomeRep *> &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<const GameOutcomeRep *> &p_outcomes)
+{
+  std::erase_if(m_outcomes, [&](const std::shared_ptr<GameOutcomeRep> &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<GameOutcomeRep> &c) mutable { c->m_number = outc++; });

src/games/gameobject.h

@@ -80,15 +80,17 @@ template <class T> 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<T> 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

src/games/gametable.cc

@@ -373,6 +373,7 @@ GameTableRep::GameTableRep(const std::vector<int> &dim, bool p_sparseOutcomes /*
     });
     std::transform(m_outcomes.begin(), m_outcomes.end(), m_results.begin(),
                    [](const std::shared_ptr<GameOutcomeRep> &c) { return c.get(); });
+    IndexOutcomeLabels();
   }
 }
 
@@ -539,7 +540,7 @@ GameTableRep::MakeOutcome(const std::vector<std::vector<GameStrategy>> &p_contin
 
   IncrementVersion();
   auto outcome = std::make_shared<GameOutcomeRep>(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]);
   }

src/games/gametree.cc

@@ -376,30 +376,27 @@ 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
 {
   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
 {
   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<GameNode> &p_nodes,
 
   IncrementVersion();
   auto outcome = std::make_shared<GameOutcomeRep>(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]);
   }

src/games/gametree.h

@@ -40,6 +40,7 @@ namespace Gambit {
 class GameActionRep : public std::enable_shared_from_this<GameActionRep> {
   friend class GameTreeRep;
   friend class GameInfosetRep;
+  friend class GameNodeRep;
   template <class T> friend class MixedBehaviorProfile;
 
   bool m_valid{true};
@@ -192,6 +193,8 @@ class GameNodeRep : public std::enable_shared_from_this<GameNodeRep> {
 
   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<GameNodeRep> {
   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
   {

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"})