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

ENH-353-initialisation of mixed profiles on creation #368

Closed

Conversation

VarsosC
Copy link

@VarsosC VarsosC commented Oct 2, 2023

No description provided.

@VarsosC VarsosC requested a review from tturocy October 2, 2023 09:44
@VarsosC VarsosC linked an issue Oct 2, 2023 that may be closed by this pull request
src/pygambit/game.pxi Outdated Show resolved Hide resolved
src/pygambit/game.pxi Outdated Show resolved Hide resolved
specifying the probabilities of the strategies.

rational
If `True`, probabilities are represented using rational numbers; otherwise
Copy link
Member

Choose a reason for hiding this comment

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

It appears that numpydoc convention is that when referring to Python constants like True, False, None and so on, not to enclose them in any back-ticks at all (as in the original version of the docstring)

src/pygambit/game.pxi Show resolved Hide resolved
@@ -7,7 +7,7 @@ def setUp(self):
"test_games/mixed_behavior_game.efg"
)
self.profile_double = self.game.mixed_behavior_profile()
self.profile_rational = self.game.mixed_behavior_profile(True)
self.profile_rational = self.game.mixed_behavior_profile(None, True)
Copy link
Member

Choose a reason for hiding this comment

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

It would be useful to add a test of creating the behavior profile set to a specific distribution, exercising the new code.

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.

Please see the individual comments for suggestions/requests.

@VarsosC
Copy link
Author

VarsosC commented Oct 5, 2023

Thanks for the suggestions and the requests.

@VarsosC VarsosC requested a review from tturocy October 17, 2023 11:39
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.

Currently this pull request has no changes at all (other than changing whether src/pygambit/game.pxi has a newline at the end of the file, and adds a superfluous import pygambit at the start of test_behav.py.

@tturocy tturocy force-pushed the ENH-353-initialisation-of-mixed-profiles-on-creation branch from 003c70f to 51e3507 Compare November 3, 2023 05:14
@tturocy tturocy force-pushed the ENH-353-initialisation-of-mixed-profiles-on-creation branch from 51e3507 to 570b8b4 Compare November 3, 2023 05:20
@tturocy tturocy added this to the gambit-16.2.0 milestone Nov 7, 2023
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.

I have made a few small comments. However, because the test suite has been rewritten for MixedBehaviorProfile the tests aren't going to merge. I would suggest making the changes on a new branch from latest master. - you should just be able to copy-paste in the changes in game.pxi and then write the test of creating with a given distribution in the new style.

@@ -8,14 +8,18 @@ def setUp(self):
self.game = gbt.Game.read_game(
"test_games/mixed_behavior_game.efg"
)
self.profile_double = self.game.mixed_behavior_profile(False)
self.profile_rational = self.game.mixed_behavior_profile(True)
self.profile_double = self.game.mixed_behavior_profile(None, rational=False)
Copy link
Member

Choose a reason for hiding this comment

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

The None here is unnecessary given that the rational= parameter name has been added

assert profile[self.game_with_chance.players[0]] == data_double[0]
assert profile[self.game_with_chance.players[1]] == data_double[1]

def test_probabilities_infoset_by_label_for_specific_distribution(self):
Copy link
Member

Choose a reason for hiding this comment

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

This test isn't necessary.

assert profile["Infoset 2:1"] == data_double[1][0]
assert profile["Infoset 3:1"] == data_double[2][0]

def test_normalize(self):
Copy link
Member

Choose a reason for hiding this comment

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

This test isn't necessary

@tturocy tturocy closed this Jan 16, 2024
@tturocy tturocy deleted the ENH-353-initialisation-of-mixed-profiles-on-creation branch February 2, 2024 09:30
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.

ENH: Initialisation of mixed profiles on creation
2 participants