Skip to content

Issue 491#504

Closed
d-kad wants to merge 7 commits intogambitproject:masterfrom
d-kad:issue_491
Closed

Issue 491#504
d-kad wants to merge 7 commits intogambitproject:masterfrom
d-kad:issue_491

Conversation

@d-kad
Copy link
Copy Markdown
Contributor

@d-kad d-kad commented Apr 11, 2025

  • add GameTreeRep::m_numNodes field and expose it in NumNodes
  • reimplement Game.nodes as a property of class Game
  • add tests for new functionality of GameNodes.__len__

solves #491

@d-kad
Copy link
Copy Markdown
Contributor Author

d-kad commented Apr 11, 2025

In test_realiz_prob_nodes_reference (test_behav.py) I had to change associations between node indices and probabilities as the traversal order for nodes has changed, seemingly due to the added dfs in GameNodes.__iter__

@tturocy should we keep it as is or should the old order be restored?

@tturocy
Copy link
Copy Markdown
Member

tturocy commented Apr 11, 2025

In test_realiz_prob_nodes_reference (test_behav.py) I had to change associations between node indices and probabilities as the traversal order for nodes has changed, seemingly due to the added dfs in GameNodes.__iter__

@tturocy should we keep it as is or should the old order be restored?

In test_realiz_prob_nodes_reference (test_behav.py) I had to change associations between node indices and probabilities as the traversal order for nodes has changed, seemingly due to the added dfs in GameNodes.__iter__

@tturocy should we keep it as is or should the old order be restored?

This is happening because of the order you're yielding nodes in the recursive iterator. Switch it so that you yield the node first then its children - this will match the previous behaviour. No need to change behaviour here unnecessarily!

This is a nice example though of how that particular test is a bit fragile and why having something like node paths will be an important improvement to develop.

@tturocy
Copy link
Copy Markdown
Member

tturocy commented Apr 17, 2025

Merged as 0a04bab.

@tturocy tturocy closed this Apr 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants