-
Notifications
You must be signed in to change notification settings - Fork 150
Add option to remove identity nodes to tikz_to_graph #401
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
base: master
Are you sure you want to change the base?
Add option to remove identity nodes to tikz_to_graph #401
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds an optional remove_identity_nodes parameter to tikz_to_graph to address issue #379, where TikZit diagrams with "none" style identity nodes cause problems in ZXLive and PyZX.
Changes:
- Splits 'none' from
synonyms_boundaryto create a separatesynonyms_nonelist for tracking identity nodes - Adds
remove_identity_nodesboolean parameter (default False) totikz_to_graphfunction - Implements logic to remove 'none' style nodes with exactly 2 neighbors and reconnect their neighbors with appropriate edge composition
- Adds comprehensive test suite covering various identity node removal scenarios
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| pyzx/tikz.py | Implements identity node removal feature with edge type composition logic and tracking of none-style vertices |
| tests/test_tikz.py | Adds 10 test cases covering simple removal, multiple nodes, edge type preservation, and edge cases |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if EdgeType.W_IO in (et1, et2): | ||
| new_et = EdgeType.W_IO | ||
| elif et1 == et2: | ||
| new_et = EdgeType.SIMPLE | ||
| else: | ||
| new_et = EdgeType.HADAMARD |
Copilot
AI
Feb 5, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The edge type composition logic may produce incorrect results when mixing W_IO edges with SIMPLE or HADAMARD edges. The current logic uses "if EdgeType.W_IO in (et1, et2): new_et = EdgeType.W_IO" which would incorrectly create a W_IO edge even when only one of the edges is W_IO and the other is SIMPLE or HADAMARD.
While this case is unlikely in practice (since identity nodes inserted into W wires should have W_IO edges on both sides), the code should either: (1) handle this case explicitly with appropriate error checking, or (2) document the assumption that both edges will be of compatible types. Consider adding a check like:
if EdgeType.W_IO in (et1, et2):
if et1 != et2:
# Mixed W_IO with non-W_IO edge - this shouldn't happen in valid diagrams
# but if it does, we should handle it explicitly
pass
new_et = EdgeType.W_IO
Or at minimum, add a comment explaining the assumption.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jvdwetering IIUC W_IO edges should only ever appear together with other W_IO edges in a valid digram, is that right? I've added a comment, but should I check for this case and throw an error or something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure. @RazinShaikh do you know?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A W_IO edge appears with a simple edge when the vertex is the W_INPUT but since this code only simplifies for none vertices, I am not sure what a valid diagram with W_IO is. What the code does currently looks reasonable to me where it converts the edges to W_IO if at least one of the two edges of the none vertex is W_IO.
pyzx/tikz.py
Outdated
| if not g.connected(n1, n2): | ||
| g.add_edge((n1, n2), new_et) |
Copilot
AI
Feb 5, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The check if not g.connected(n1, n2) prevents adding the composed edge when the neighbors are already connected. This is incorrect because it doesn't account for edge composition according to ZX-calculus rules.
For example, if there's a configuration like:
- n1 --HADAMARD--> identity_node --HADAMARD--> n2 (composition yields SIMPLE)
- n1 --SIMPLE--> n2 (existing direct edge)
After removing identity_node, the code should add the composed SIMPLE edge to the existing connection. The backend's add_edge method would then apply the appropriate ZX-calculus rules to reduce the parallel edges. However, the current connected check prevents this from happening.
The fix is to remove the if not g.connected(n1, n2): check and always call g.add_edge((n1, n2), new_et). The backend will handle parallel edge reduction correctly.
| if not g.connected(n1, n2): | |
| g.add_edge((n1, n2), new_et) | |
| g.add_edge((n1, n2), new_et) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
tests/test_tikz.py
Outdated
| def test_none_node_removal_avoids_multi_edge(self): | ||
| """Removing a 'none' node doesn't create a multi-edge if neighbours are already connected.""" | ||
| tikz = r'''\begin{tikzpicture} | ||
| \begin{pgfonlayer}{nodelayer} | ||
| \node [style=z spider] (0) at (0, 0) {}; | ||
| \node [style=none] (1) at (1, 0) {}; | ||
| \node [style=z spider] (2) at (2, 0) {}; | ||
| \end{pgfonlayer} | ||
| \begin{pgfonlayer}{edgelayer} | ||
| \draw (0) to (1); | ||
| \draw (1) to (2); | ||
| \draw (0) to (2); | ||
| \end{pgfonlayer} | ||
| \end{tikzpicture}''' | ||
| g = tikz_to_graph(tikz, warn_overlap=False, remove_identity_nodes=True) | ||
| # The 'none' node is removed but no duplicate edge is added. | ||
| self.assertEqual(g.num_vertices(), 2) | ||
| self.assertEqual(g.num_edges(), 1) |
Copilot
AI
Feb 5, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test doesn't adequately verify edge composition when neighbors are already connected. The test uses all SIMPLE edges between same-color Z spiders, where adding a parallel SIMPLE edge is a no-op according to spider fusion rules. This means the test passes even though there's a bug in the implementation (see the comment on line 480).
Consider adding a test case where the edge composition matters, such as:
- SIMPLE edge through identity node + HADAMARD edge direct = should compose correctly
- HADAMARD + HADAMARD through identity node + HADAMARD edge direct = should apply Hopf law
For example, a test with nodes (0: Z spider) --SIMPLE--> (1: none) --HADAMARD--> (2: Z spider) and a direct HADAMARD edge from 0 to 2. After removing node 1, the composed HADAMARD edge should be added to the existing HADAMARD edge, resulting in edge removal by the Hopf law.
1132ef1 to
b6dd181
Compare
b6dd181 to
28b2d8b
Compare
Fixes #379.