Repository navigation
Remove friend access to AGG model from gametracer - #1034
Merged
Merged
Conversation
This removes a dubious situation in which gametracer's `aggame` had friend access to the AGG class, and further used that for per-run algorithm state. The friend access was being used for a more efficient calculation fo the Jacobian. This commit replaces that with a non-optimised version using only public API; the spirit of the optimised version will be re-instated in a subsequent commit.
Adds AGG::getPayoffJacobianRow, computing a full row of the payoff Jacobian via a leave-one-out convolution instead of one independent getJ call per (column player, action) pair. Two changes from the original: * The pure-node division trick fast path has not been replicated * Cross-row memoisation is not implemented, to avoid shared mutable state across calls.
There was a problem hiding this comment.
Pull request overview
Removes gametracer’s privileged AGG access while adding exact Rational payoff processing.
Changes:
- Adds a public row-wise payoff Jacobian API for gametracer.
- Generalizes AGG/BAGG payoff computation to
doubleandRational. - Adds exact fraction and decimal fixtures and tests.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
tests/test_nash.py |
Documents exact AGG solver behavior. |
tests/test_games/2x2_fraction_types.bagg |
Adds fractional BAGG probabilities. |
tests/test_games/2x2_fraction_payoffs.agg |
Adds exact fractional/decimal payoffs. |
tests/test_agg.py |
Tests exact AGG/BAGG calculations. |
src/tools/gt/nfgipa.cc |
Removes unused gametracer namespace access. |
src/solvers/gtracer/aggame.h |
Uses public AGG APIs. |
src/solvers/gtracer/aggame.cc |
Replaces internal Jacobian implementation. |
src/games/number.h |
Adds textual number streaming. |
src/games/gamebagg.h |
Uses exact BAGG extrema. |
src/games/gamebagg.cc |
Enables Rational BAGG profiles. |
src/games/gameagg.h |
Uses exact AGG extrema. |
src/games/gameagg.cc |
Enables Rational AGG profiles. |
src/games/agg/trie_map.imp |
Generalizes numeric constants. |
src/games/agg/trie_map.cc |
Instantiates double/Rational tries. |
src/games/agg/bagg.h |
Adds exact BAGG data and APIs. |
src/games/agg/bagg.cc |
Implements exact BAGG calculations. |
src/games/agg/agg.h |
Defines public generic AGG interfaces. |
src/games/agg/agg.cc |
Implements exact payoffs and row Jacobians. |
Suppressed comments (1)
src/games/agg/agg.cc:1093
- The scan starts at action node 1, so for node 0 only
begin()->secondis considered and every other payoff entry is ignored. A game whose minimum is a later configuration of its first action node will report the wrongmin_payoff; include node 0 in the loop.
for (int i = 1; i < numActionNodes; i++) {
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
|
||
| std::vector<int> typeIndex(others.size(), 0); | ||
| Rational total(0); | ||
| while (true) { |
Comment on lines
+161
to
+163
| template <class V> | ||
| void getPayoffJacobianRow(int player1, int action1, const StrategyProfile<V> &s, | ||
| StrategyProfile<V> &dest); |
Comment on lines
+616
to
+617
| computeAllButOne(others, size_t{0}, others.size(), base, numNei, projFunctions[Node], allBut, | ||
| temp); |
Comment on lines
+218
to
+221
| // Constructing the profile itself is fine (e.g. to exactly represent a pure-strategy | ||
| // profile as a degenerate mixed one, as EnumPureStrategySolve does) -- it's specifically | ||
| // *payoff computation* on it that AGG cannot do exactly; see | ||
| // AGGMixedStrategyProfileRep<Rational>::GetPayoff. |
Comment on lines
+290
to
+293
| // Constructing the profile itself is fine (e.g. to exactly represent a pure-strategy | ||
| // profile as a degenerate mixed one, as EnumPureStrategySolve does) -- it's specifically | ||
| // *payoff computation* on it that BAGG cannot do exactly; see | ||
| // BAGGMixedStrategyProfileRep<Rational>::GetPayoff. |
Comment on lines
+67
to
+71
| profile[s0[0]] = gbt.Rational(1) | ||
| profile[s0[1]] = gbt.Rational(0) | ||
| profile[s1[0]] = gbt.Rational(0) | ||
| profile[s1[1]] = gbt.Rational(1) | ||
| assert profile.payoff(p0) == gbt.Rational(1, 3) |
# Conflicts: # src/games/agg/agg.cc # src/games/agg/agg.h # src/games/agg/bagg.cc # src/games/agg/bagg.h # src/games/gamebagg.cc # src/solvers/gtracer/aggame.cc # src/solvers/gtracer/aggame.h # tests/test_agg.py # tests/test_nash.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This removes access for game tracer to the private parts of the AGG model.
The substantive change is to re-write the more efficient payoff-Jacobian calculation without per-call shared state or direct access to AGG internals.
This depends on PR #1029, as doing this independently would simply lead to a bunch of merge conflicts to resolve!