Import a list of valid terminal node references. #438

VarsosC · 2024-01-18T11:35:56Z

Import a list of valid terminal node references for the append_move and append_infoset functions. Further, the test_node and test_infosets test function have been modified to check the newly acquired functionality.

tturocy · 2024-01-19T11:12:24Z

src/pygambit/game.pxi

@@ -1018,6 +1018,9 @@ class Game:
            f"{funcname}(): {argname} must be Node or str, not {node.__class__.__name__}"
        )

+        if not isinstance(argname, (Node, str)) and not hasattr(argname, "__iter__"):


This is unreachable code, and actually is not needed.

tturocy · 2024-01-19T11:12:53Z

src/pygambit/game.pxi

@@ -1100,47 +1103,79 @@ class Game:
            f"{funcname}(): {argname} must be Action or str, not {action.__class__.__name__}"
        )

-    def append_move(self, node: typing.Union[Node, str],
+    NodeReference = typing.Union[Node, str]


Defining these here means they are members of the class Game. They should be defined at global scope in an appropriate place.

tturocy · 2024-01-19T11:14:28Z

src/pygambit/game.pxi

+        else:
+            nodes = [nodes]
+
+        resolved_nodes = [self._resolve_node(nd, "append_move") for nd in nodes]


It would be better to write this as one line:

resolved_nodes = [self._resolve_node(n, "append_move") for n in nodes if hasiter(nodes, "__iter__") else [nodes]]

Rationale: This way the type of nodes does not get changed.

src/pygambit/game.pxi

tturocy · 2024-01-19T11:17:08Z

src/pygambit/game.pxi

        resolved_node.node.deref().AppendMove(resolved_player.player, len(actions))
        for label, action in zip(actions, resolved_node.infoset.actions):
            action.label = label

-    def append_infoset(self, node: typing.Union[Node, str],
+        infoset = resolved_node.infoset
+        resolved_infoset = cython.cast(Infoset, self._resolve_infoset(infoset, "append_infoset"))


There is no need to call _resolve_infoset here because resolved_node.infoset is already an Infoset. (The argument to _resolve_infoset identifying the function is also incorrect but that is moot.)

tturocy · 2024-01-19T11:23:45Z

tests/test_infosets.py

@@ -52,3 +60,47 @@ def test_infoset_add_action_error():
    game = games.read_from_file("basic_extensive_game.efg")
    with pytest.raises(gbt.MismatchError):
        game.add_action(game.players[0].infosets[0], game.players[1].infosets[0].actions[0])
+
+def test_append_move_single_infoset_ist_of_nodes():


spelling error

tturocy · 2024-01-19T11:24:23Z

tests/test_infosets.py

+             game.root.children[0].children[1]]
+    game.append_move(nodes, "Player 3", ["B", "F"])
+
+    assert len(game.players[2].infosets) == 1


It would be more transparent to be consistent and index player by label in the assertion.,

tturocy · 2024-01-19T11:24:53Z

tests/test_infosets.py

+
+def test_append_move_infoset_members_list_of_nodes():
+    """Test that the newly created infoset from a list of nodes is inherited
+    with as many members as the cardinality of the list.


Would not a better test be to confirm that the correct nodes are in fact in the same information set, rather than just the length is correct?

tturocy · 2024-01-19T11:25:14Z

tests/test_infosets.py

+                                game.root.children[0].children[0].infoset)
+
+
+def test_append_infoset_node_list_with_duplciate_nodes():


spelling error

tests/test_node.py

tturocy

This still is failing on some very simple cases.

The works in master (and should work as it is correct):

In [1]: import pygambit as gbt

In [2]: g = gbt.Game.new_tree(players=["Alice", "Bob"])

In [3]: g.root.label = "ROOT"

In [4]: g.append_move("ROOT", "Alice", ["L", "R"])
---------------------------------------------------------------------------
KeyError                                  Traceback (most recent call last)
Cell In[4], line 1
----> 1 g.append_move("ROOT", "Alice", ["L", "R"])

File src/pygambit/game.pxi:1124, in pygambit.gambit.Game.append_move()

File src/pygambit/game.pxi:1016, in pygambit.gambit.Game._resolve_node()

KeyError: "append_move(): no node with label 'R'"

This produces an incorrect error because the checks for duplicate nodes are not correct.

In [1]: import pygambit as gbt

In [2]: g = gbt.Game.new_tree(players=["Alice", "Bob"])

In [3]: g.root.label = "ROOT"

In [4]: g.append_move(["ROOT", g.root], "Alice", ["L", "R"])
---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
Cell In[4], line 1
----> 1 g.append_move(["ROOT", g.root], "Alice", ["L", "R"])

File src/pygambit/game.pxi:1146, in pygambit.gambit.Game.append_move()

ValueError: Undefined operation on game

tturocy

Please see comments.

For future please ensure that the pull request contains just one commit (i.e. it's been rebased and squashed), so that if it is acceptable it's merge-ready.

tturocy · 2024-01-31T11:24:55Z

src/pygambit/game.pxi

@@ -1122,14 +1122,14 @@ class Game:

        resolved_nodes = (
                       [self._resolve_node(n, "append_move") for n in nodes]
-                       if hasattr(nodes, "__iter__")
+                       if (hasattr(nodes, "__iter__") and not isinstance(nodes, str))


The parentheses are redundant.

tturocy · 2024-01-31T11:25:21Z

src/pygambit/game.pxi

                       else [self._resolve_node(nodes, "append_move")]
        )

        if any(len(rn.children) > 0 for rn in resolved_nodes):
            raise UndefinedOperationError("append_move(): `node` must be a terminal node")

-        if hasattr(nodes, "__iter__") and (len(nodes) != len(set(nodes))):
+        if hasattr(nodes, "__iter__") and any(nodes.count(n) > 1 for n in nodes):


This misses the point of the previous error cases.

The error here is that resolved_node should be used, not nodes.

tturocy · 2024-01-31T11:31:58Z

src/pygambit/game.pxi

                       else [self._resolve_node(nodes, "append_move")]
        )

        if any(len(rs.children) > 0 for rs in resolved_nodes):
            raise UndefinedOperationError("append_move(): `node` must be a terminal node")

-        if hasattr(nodes, "__iter__") and len(nodes) != len(set(nodes)):
+        if hasattr(nodes, "__iter__") and any(nodes.count(n) > 1 for n in nodes):
            raise ValueError("append_move(): Each `node` must be listed only once.")

        if hasattr(nodes, "__iter__") and (len(nodes) == 0):


Likewise multiple issues:

The parentheses are redundant;

However, this check should be on resolved_nodes instead of nodes in which case checking for whether it's iterable is redundant;

Then the correct way (per PEP 8) to check whether a list is empty is using not, not len() == 0.

tturocy · 2024-02-02T09:23:04Z

src/pygambit/game.pxi

        resolved_player = cython.cast(Player, self._resolve_player(player, "append_move"))
-        if len(resolved_node.children) > 0:
-            raise UndefinedOperationError("append_move(): `node` must be a terminal node")
        if len(actions) == 0:


len() == 0 is not Pythonic - should use not actions

tturocy · 2024-02-02T09:23:38Z

src/pygambit/game.pxi

        if len(actions) == 0:
            raise UndefinedOperationError("append_move(): `actions` must be a nonempty list")
+
+        resolved_nodes = (


The outer parentheses are redundant. You can just use the square brackets for the list directly.

tturocy · 2024-02-02T09:25:32Z

src/pygambit/game.pxi

+        if any(len(n.children) > 0 for n in resolved_nodes):
+            raise UndefinedOperationError("append_move(): `node` must be a terminal node")
+
+        if any(resolved_nodes.count(n) > 1 for n in resolved_nodes):


This is quadratic - .count() is linear in the length of the list and then you're iterating the list.

Much more straightforward and more efficient is len(resolved_nodes) != len(set(resolved_nodes))

…multaneous action.

VarsosC requested a review from tturocy January 19, 2024 08:35

VarsosC linked an issue Jan 19, 2024 that may be closed by this pull request

ENH: Atomic add of move at multiple nodes #366

Closed

tturocy requested changes Jan 19, 2024

View reviewed changes

tturocy requested changes Jan 25, 2024

View reviewed changes

tturocy requested changes Jan 31, 2024

View reviewed changes

VarsosC force-pushed the ENH-366-incorporating-lists-of-node-references-for-various-calls branch from 937721f to 3d3b125 Compare February 2, 2024 07:27

tturocy requested changes Feb 2, 2024

View reviewed changes

VarsosC force-pushed the ENH-366-incorporating-lists-of-node-references-for-various-calls branch from 3d3b125 to ffa5792 Compare February 2, 2024 13:00

tturocy force-pushed the ENH-366-incorporating-lists-of-node-references-for-various-calls branch from ffa5792 to c23c4bf Compare February 8, 2024 17:25

Extend append_move and append_infoset to accept lists of nodes for si…

4ab6060

…multaneous action.

tturocy force-pushed the ENH-366-incorporating-lists-of-node-references-for-various-calls branch from c23c4bf to 4ab6060 Compare February 8, 2024 17:31

tturocy approved these changes Feb 8, 2024

View reviewed changes

tturocy merged commit 3e9a9d6 into master Feb 9, 2024
19 checks passed

tturocy deleted the ENH-366-incorporating-lists-of-node-references-for-various-calls branch February 9, 2024 09:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Import a list of valid terminal node references. #438

Import a list of valid terminal node references. #438

VarsosC commented Jan 18, 2024

tturocy Jan 19, 2024

tturocy Jan 19, 2024

tturocy Jan 19, 2024

tturocy Jan 19, 2024

tturocy Jan 19, 2024

tturocy Jan 19, 2024

tturocy Jan 19, 2024

tturocy Jan 19, 2024

tturocy left a comment

tturocy left a comment

tturocy Jan 31, 2024

tturocy Jan 31, 2024

tturocy Jan 31, 2024

tturocy Feb 2, 2024

tturocy Feb 2, 2024

tturocy Feb 2, 2024

		game.root.children[0].children[0].infoset)


		def test_append_infoset_node_list_with_duplciate_nodes():

Import a list of valid terminal node references. #438

Import a list of valid terminal node references. #438

Conversation

VarsosC commented Jan 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tturocy left a comment

Choose a reason for hiding this comment

tturocy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment