Skip to content
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

Initialise the MixedBehaviorProfile on creation. Add test to the new … #435

Conversation

VarsosC
Copy link

@VarsosC VarsosC commented Jan 15, 2024

…test suite that checks the creation of a behavior profile given a distribution.

for each player over his actions.
"""
profile = game.mixed_behavior_profile(rational=rational_flag, data=data)
for p in range(len(game.players)):
Copy link
Member

Choose a reason for hiding this comment

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

We are trying to avoid using integer indices as it's not very Pythonic. Better would be:

for (player, behavior) in zip(game.players, data):
    assert profile[player] == behavior

@@ -641,3 +645,22 @@ def test_action_value_with_chance_player_action(game: gbt.Game, rational_flag: b
chance_action = game.players.chance.infosets[0].actions[0]
with pytest.raises(ValueError):
game.mixed_behavior_profile(rational=rational_flag).action_value(chance_action)


@pytest.mark.parametrize(
Copy link
Member

Choose a reason for hiding this comment

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

This only tests with data that has the correct "shape". There are no tests to ensure that bad input data is handled appropriate (with an error).

Copy link
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.

See comments. Please ensure that PRs are a single comment ready to rebase against master if approved.

(games.create_complicated_extensive_game(), False, [[[1/5, 4/5], [3/5, 2/5]], [[1/4, 3/4]]])
]
)
def test_specific_profile(game: gbt.Game, rational_flag: bool, data: float):
Copy link
Member

Choose a reason for hiding this comment

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

Type hint for data is not correct.

[[1/2, 1/2]])
]
)
def test_profile_data_error(game: gbt.Game, rational_flag: bool, data: float):
Copy link
Member

Choose a reason for hiding this comment

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

Type hint for data is not correct.

[["4/9", "5/9"], ["1/3", "2/3"]])
]
)
def test_tree_representation_error(game: gbt.Game, rational_flag: bool, data: float):
Copy link
Member

Choose a reason for hiding this comment

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

Type hint for data is not correct.

number of actions per infoset.
"""
with pytest.raises(ValueError):
game.mixed_behavior_profile(rational=rational_flag,data=data)
Copy link
Member

Choose a reason for hiding this comment

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

We prefer a space between function arguments (this is done consistently everywhere else)

@VarsosC VarsosC force-pushed the ENH-353-initialisazion-and-testing-mixed-profiles-on-creation branch from 4158424 to 01e0355 Compare February 2, 2024 08:39
@pytest.mark.parametrize(
"game,rational_flag,data",
[(games.create_mixed_behav_game(), True,
[[[gbt.Rational("0/1"), gbt.Rational("1/1"), gbt.Rational("0/1")]],
Copy link
Member

Choose a reason for hiding this comment

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

Mixed profiles accept anything that can be interpreted as a number as an argument. Therefore you do not need gbt.Rational("0/1") - you can just write "0/1", and so on. But for whole numbers you can just do 0 and 1.

@@ -768,10 +773,50 @@ class Game:
if not rational:
Copy link
Member

Choose a reason for hiding this comment

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

In this case, now that so much code has been added to this branch of the if self.is_tree, it would be much better to reverse the conditional. That is, have a conditional that checks if not self.is_tree first. Then, you do not need an else after that. This can be seen by imitating the implementation of mixed_strategy_profile which is immediately adjacent in the source file.

@VarsosC VarsosC force-pushed the ENH-353-initialisazion-and-testing-mixed-profiles-on-creation branch from 01e0355 to 75985ee Compare February 3, 2024 12:51
@tturocy tturocy force-pushed the ENH-353-initialisazion-and-testing-mixed-profiles-on-creation branch 3 times, most recently from 39ccd09 to 13a6225 Compare February 8, 2024 16:34
@tturocy tturocy force-pushed the ENH-353-initialisazion-and-testing-mixed-profiles-on-creation branch from 13a6225 to 2a22cec Compare February 8, 2024 16:47
@tturocy tturocy merged commit 05bdbfa into master Feb 8, 2024
19 checks passed
@tturocy tturocy deleted the ENH-353-initialisazion-and-testing-mixed-profiles-on-creation branch February 8, 2024 16:56
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.

None yet

2 participants