Skip to content

Conversation

@d-kad
Copy link
Contributor

@d-kad d-kad commented May 22, 2025

Implement, for a given action, its veto

Considering the implementation details of strategy.power, it appears optimal to define first strategy.veto as the union of vetos of corresponding actions the strategy assigns and then generate strategy.power as its complement.

Thus, action.power appears to be redundant.

@d-kad d-kad requested review from tturocy May 22, 2025 10:27
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've written a few implementation comments. However, I am coming back conceptually to the comments I made after your talk a few months ago. I'm now not sure that power (defined like this) is the concept we want, but rather its complement.

Also as a tip: The "Closes #XXX" needs to be in the commit message to auto-close an issue, not in the covering description of the PR.

@d-kad d-kad self-assigned this Jun 6, 2025
Action.precedes
Action.prob
Action.plays
Action.power
Copy link
Member

Choose a reason for hiding this comment

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

This should be Action.veto

@property
def plays(self) -> typing.List[Node]:
"""Returns a list of all terminal `Node` objects consistent with it.
"""Returns a list of all terminal `Node` objects consistent with the action.
Copy link
Member

Choose a reason for hiding this comment

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

Simpler is just to say 'terminal nodes' rather than 'terminal Node objects'

Node.wrap(n) for n in
self.action.deref().GetInfoset().deref().GetGame().deref().GetPlays(self.action)
]

Copy link
Member

Choose a reason for hiding this comment

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

Docstring has not been updated.

It would be useful to write a brief explanation of what the 'veto' of an action is here - a few lines so the documentation is self-contained

assert set(test_action.plays) == expected_set_of_plays


def test_action_veto():
Copy link
Member

Choose a reason for hiding this comment

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

This explanation would be better put in the docstring of veto rather than in the tests.



def test_action_veto():
"""Verify `action.veto` returns the action's veto
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 have a few test fixtures rather than a single test (see other examples throughout the test suite for setting up fixtures). In particular think about potential edge cases - e.g. actions at the root node, actions at singleton information sets versus nontrivial information sets, so on.

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