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

BUG: Chance action probabilities after deleting chance action #372

Closed
tturocy opened this issue Oct 6, 2023 · 3 comments · Fixed by #395
Closed

BUG: Chance action probabilities after deleting chance action #372

tturocy opened this issue Oct 6, 2023 · 3 comments · Fixed by #395
Assignees
Labels
c++ Items which involve writing in C++ python Items which involve coding in Python
Milestone

Comments

@tturocy
Copy link
Member

tturocy commented Oct 6, 2023

Description

Since #303, action probabilities at chance information sets are enforced to sum to one on edit. However, the change did not take into account that it is possible to delete actions at chance information sets - which in general would result in probability distributions at that information set which do not sum to one.

Possible solutions

There are a few options to be considered.

#. The action probabilities of the remaining actions can be renormalised proportionally. This makes some sense, but has some drawbacks. (a) The game representation remembers the representation of the action probabilities as either decimal or rational numbers; renormalising could result in a change in the format. (b) The renormalisation would take place silently, and it might not be the user's intention.

#. Only allow zero-probability actions to be deleted. This would make the user set probabilities explicitly. However, it does leave the possibility of the zero-probability actions remaining. This should still be valid, but it does cause complications for some equilibrium calculation methods (see #63 for example).

#. The add and delete actions operations could be coalesced into a single operation, which for the moment let's call set_actions. For a chance move, set_actions(infoset, actions) would require a mapping from actions to probabilities, where actions as usual could be specified by label or by action object (requiring the action to be an action at that information set). New actions could be added by introducing a new label, and actions deleted by dropping the label. This would also allow actions to be re-ordered - re-ordering the actions would result in the corresponding subtrees being reordered. For personal player moves, actions would just be a list instead of a mapping. This behaviour is similar to using DataFrame.reindex in pandas, where it's possible to add, delete, and reorder rows and columns arbitrarily.

Of these, the last one seems attractive, as it unifies two existing operations, and further makes it easy to do another (reordering) which, while possible to do now, is not at all straightforward.

@tturocy tturocy added this to the gambit-16.1.0 milestone Oct 6, 2023
@tturocy tturocy added python Items which involve coding in Python c++ Items which involve writing in C++ labels Oct 6, 2023
@rahulsavani
Copy link
Member

@tturocy: I'd be willing to take this one on if we, say, agree that the third option is the one to go for (which seems very reasonable to me).

@tturocy
Copy link
Member Author

tturocy commented Oct 11, 2023

@rahulsavani Having given it some thought, I propose we do the following:

  1. For 16.1, we will have .delete_action() simply renormalise to sum-to-one. We are making a change that enforces sum-to-one when changing probabilities, and so it seems OK also to change the behaviour on this operation as well. The downside that it might change representation from decimal to rational is a small problem by comparison - the result will still be correct mathematically even if the representation is changed.

  2. For 16.2, we plan to proceed along the lines of the third option. That gives us a bit more time to think it through - we can put in deprecations in 16.2 for 16.3 for the old methods, and also we can look and see whether any other operations ought to be re-phrased in a similar pattern.

If all are agreed, then for this issue, let's fix it for 16.1 by renormalising - as that will address the BUG part of this - and I'll create a new issue for 16.2 as an enhancement for implementing the .set_actions() idea.

@tturocy
Copy link
Member Author

tturocy commented Oct 11, 2023

Per the above discussion #378 now marks the idea of evolving the API in 16.2.x.

The change in this issue should be implemented at the C++ level so it also applies correctly to the GUI.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Items which involve writing in C++ python Items which involve coding in Python
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants