-
Notifications
You must be signed in to change notification settings - Fork 168
Implement efficient single-pass subgame root finder to replace the legacy IsSubgameRoot
#664
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
tturocy
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In addition to the comments within (which are already substantial enough to leave it where it is for now and iterate) - doesn't it make sense also to be replacing the implementation of IsSubgameRoot as well as part of this?
tturocy
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This still requires a lot more thoughts about data structures, and about STL-style generic programming to avoid doing a lot of manual book-keeping.
df5d91a to
4002cb3
Compare
…) of generated components
…s; remove overrides from other child classes
… edge case of the empty game; add documentation
tturocy
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comments interspersed.
src/games/gametree.cc
Outdated
| // Comparator for priority queue (ancestors have lower priority than descendants) | ||
| // This ensures descendants are processed first, allowing upward traversal to find | ||
| // the highest reachable node in each component | ||
| struct NodeCmp { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From a C++ perspective, can't we just get away with a lambda in the definition of a priority queue, without having to go through the palaver of a struct with call defined?
The problem with relying on numbers here is that nodes are only numbered in SortInfosets. And you're not calling SortInfosets. So your tests are only working by accident, because you are only working with games that have been loaded from a file (and those are automatically sorted).
We really need to replace this with something which does not require all nodes to be numbered at the start. Otherwise we are making yet another pass through the tree to do this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rahulsavani @tturocy In this version, I added a local pre-pass to number the nodes. This removed the dependency on SortInfosets (triggered by BuildComputedValues) while keeping numbers used by the algorithm.
For context on the comparator: I attempted to use node depth to avoid the pre-pass, but this resulted in 6 test failures. The order of nodes on the local frontier was arbitrary when they had the same depth, breaking the merging logic (more on this and lexicographic comparison of nodes' action paths in my comment below).
|
@tturocy @rahulsavani Would this be a convincing property? Technically: We pop the node with the highest DFS preorder number from the local frontier. Structurally: This means processing the deepest, leftmost node in the local frontier at each step, its lower-left corner. Formal characterization: Consider the path from the root to a node as a sequence of action indices. Each time a node is popped from the local frontier, it has the lexicographically maximal path among all nodes on this frontier, defined by the rules that (i) if two paths diverge at a common ancestor, the path taking the left-most branch is strictly greater, ignoring any difference in global depth, and (ii) if one path is a subset of another, the longer path is considered greater. Why not use lexicographic path comparison directly? Drawbacks:
Pre-computing DFS numbers may thus be an optimal choice: it trades one upfront tree traversal with constant comparisons for the same tree traversal with storing paths (more memory intensive than storing numbers) and expensive lexicographic comparisons. |
|
I am not convinced. While it is not a formal argument, I have never seen a graph algorithm that starts with "make a copy of the graph." I suggest the next step needs to be that we need to write this algorithm down carefully in a draft manuscript alongside its correctness argument - because I am also not fully convinced that even assuming we have node numbers that the rest of the algorithm is not doing unnecessary work. (It may well not be doing so but without writing it down formally we can be getting muddled between what the algorithm does and what we need to do in C++ to implement it.) |
Description
This PR replaces the legacy implementation of
IsSubgameRootwith a new single-pass algorithm.Computes and caches subgame roots for all infosets in one traversal.
Key Changes
FindSubgameRootsusing bottom-up traversal (DSU + Priority Queue)GenerateComponentandSubgameScratchDatahelpers in anonymous namespace.BuildSubgameRootsto populate and cachem_infosetSubgameRootCloses #584