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

372 bug chance action probabilities after deleting chance action #395

Conversation

rahulsavani
Copy link
Member

@rahulsavani rahulsavani commented Oct 26, 2023

This is now a single commit with a ChangeLog entry, and is so ready for review and possible merging

@rahulsavani rahulsavani requested a review from tturocy October 26, 2023 13:49
@rahulsavani rahulsavani linked an issue Oct 26, 2023 that may be closed by this pull request
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 the one improvement in terms of localising this change to TreeGameRep alone. Otherwise this looks ready to merge - if you can squash and force-push first that'd certainly be helpful!

src/games/game.h Outdated
@@ -847,6 +847,8 @@ class GameRep : public BaseGameRep {
//@{
/// Set the probability distribution of actions at a chance node
virtual Game SetChanceProbs(const GameInfoset &, const Array<Number> &) = 0;
/// Normalize the probability distribution of actions at a chance node
Copy link
Member

Choose a reason for hiding this comment

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

Having a separate NormaliseChanceProbs function makes sense, but it doesn't need to be a member of GameRep. It just needs to be present in TreeGameRep as that's the only representation (at least at the moment) that implements the operation. That means changes to the other representation classes can be rolled back.

Further, within TreeGameRep, this should be a private member function; there's no reason for external code to call this as having the probabilities be summing to one should already be maintained as an invariant.

@rahulsavani rahulsavani force-pushed the 372-bug-chance-action-probabilities-after-deleting-chance-action branch from 0f57236 to 7c6c614 Compare October 27, 2023 12:20
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.

You've accidentally added the .idea config files in the commit. We do actually need to have a conversation about which ones we should include - because these can control things like static code checkers and formatters using those. But that's something for another time!

@rahulsavani rahulsavani force-pushed the 372-bug-chance-action-probabilities-after-deleting-chance-action branch from 2a5810d to 7f06268 Compare October 29, 2023 17:43
@rahulsavani rahulsavani force-pushed the 372-bug-chance-action-probabilities-after-deleting-chance-action branch from 7f06268 to c7d8116 Compare October 29, 2023 17:45
@rahulsavani
Copy link
Member Author

You've accidentally added the .idea config files in the commit. We do actually need to have a conversation about which ones we should include - because these can control things like static code checkers and formatters using those. But that's something for another time!

Yep I did a git add . a one point and apparently didn't clear up fully. That's been dealt with now and squashed.

@tturocy tturocy merged commit 289efbd into master Oct 30, 2023
15 checks passed
@tturocy tturocy deleted the 372-bug-chance-action-probabilities-after-deleting-chance-action branch October 30, 2023 13:25
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.

BUG: Chance action probabilities after deleting chance action
2 participants