Improve testing of do, and fix various quirks in implementation

[mscroggs]

• Improve unit testing of do: resolves Improve unit testing of do() #55
  • One of the new tests asserts that the application of do in Work out what to do when calling do on a node with shared predecessors #80 raises an error. We should update this test once we decide what the sensible behaviour for this graph is.
• Fix issues in implementation of do that new tests revealed
• Refactor do to work with list of new nodes rather then adding and removing nodes and edges from deep copy of the networkx graph

[willGraham01]

Suggested change

	def get_included_excluded_successors(
	graph: Graph, nodes: dict[str, Node], node: str
	) -> tuple[list[str], list[str]]:
	"""
	Get list of successors that are included in and excluded from nodes dictionary.
	Args:
	graph: The graph
	nodes: A dictionary of nodes, indexed by label
	node: The node to check the successors of
	Returns:
	Lists of included and excluded nodes
	"""
	def get_included_excluded_successors(
	graph: Graph, nodes_to_split: dict[str, Node], successors_of: str
	) -> tuple[list[str], list[str]]:
	"""
	Split nodes into two groups; those that are successors or another node, and those that are not.
	Args:
	graph: The graph
	nodes_to_split: A dictionary of nodes, indexed by label
	successor_of: The node to check the successors of
	Returns:
	Lists of included and excluded nodes
	"""

It took me a few tries to figure out what was going on here, until I saw the variable names and what was going on in the function. I've suggested a change to the docstring and variable names which I think makes it a bit clearer what's going on? (You'll have to change the variable names in the function though, sorry):

• nodes $\rightarrow$ nodes_to_split
• node $\rightarrow$ successors_of

[willGraham01]

(Will also add here that return types could also be tuple of tupless)

[mscroggs]

I updated the docstring: it splits the successors of the node into nodes in the list and not in the list (instead of splitting the nodes into successors and non-successors). The had to make it less long as ruff got annoyed

[willGraham01]

More a comment here for my own understanding, but both removable_nodes and get_incluced_excluded_nodes are only be needed if we want to retain some numpyro-independent implementation of do right (in case we go back to the backend-agnostic stuff)? If we are only wanting to use numpryo, we could just do this for the time being (assuming the other caveats).

Would be an interesting test case to compare our do and this one, regardless.

[mscroggs]

I've opened #91 to remind me that this is a test we want