Skip to content

Add tests to verify the new SubgameRootFinder prototype#611

Closed
d-kad wants to merge 1 commit intogambitproject:masterfrom
d-kad:issue_583
Closed

Add tests to verify the new SubgameRootFinder prototype#611
d-kad wants to merge 1 commit intogambitproject:masterfrom
d-kad:issue_583

Conversation

@d-kad
Copy link
Copy Markdown
Contributor

@d-kad d-kad commented Nov 13, 2025

The added tests verify the new SubgameRootFinder Python prototype against the legacy is_subgame_root logic.

The new algorithm correctly identifies all legacy subgame roots, and this test suite confirms that any additional roots it finds are exclusively members of absent-minded information sets, which the legacy method was designed to exclude.

against the legacy `is_subgame_root`
@d-kad d-kad requested a review from tturocy November 13, 2025 11:54
Copy link
Copy Markdown
Member

@tturocy tturocy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't the way to go about this. We shouldn't add complicated code to the test suite, even if temporarily, because this confounds the test (what if the complicated testing code is itself wrong?)

Instead there is a much more straightforward approach. Write tests for is_subgame_root which simply test whether the node in question should or should not be a subgame root, against an expected value (which expected value we provide, we don't compute it).

We expect the existing is_subgame_root implementation is correct in all cases except one - the one in which there is an absent-minded information set which does not have as one of its members the root of the tree.

We should add a test for such a case as well. That test will fail currently. That is OK. We mark that test with @pytest.mark.xfail (see https://docs.pytest.org/en/stable/how-to/skipping.html#skipping).

Then, when we implement the new algorithm, that test will start passing because it will correct this bug. But that will actually mean the test will be considered by pytest to fail! But that will be good (and at that time we remove the xfail mark from the test and all manner of thing shall be well.

@rahulsavani

@tturocy
Copy link
Copy Markdown
Member

tturocy commented Nov 13, 2025

Because a fundamentally different approach is needed, closing this one.

@tturocy tturocy closed this Nov 13, 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