Skip to content

replace ground-based Reset with symbolic boolean paradigm#430

Open
dlyongemallo wants to merge 1 commit intozxcalc:masterfrom
dlyongemallo:check_copy_steane_circuit
Open

replace ground-based Reset with symbolic boolean paradigm#430
dlyongemallo wants to merge 1 commit intozxcalc:masterfrom
dlyongemallo:check_copy_steane_circuit

Conversation

@dlyongemallo
Copy link
Copy Markdown
Contributor

@dlyongemallo dlyongemallo commented Mar 30, 2026

Description

Replaces the ground-based representation of Reset with a symbolic-boolean representation.

Previously, Reset produced a Z(0) spider with ground=True (trace-out) followed by a fresh |0> boundary, which mixed two paradigms. This caused copy_simp (in full_reduce) to copy a measurement's symbolic-phase X leaf through the adjacent reset ground, destroying the measurement outcome.

After this change, Reset will produce a Z(0) spider with an X(_rN) leaf hanging off of it (where _rN is a fresh boolean variable), with an outcome_type='reset_discard' tagged in vertex data (distinct from outcome_type='measurement'), followed by a disconnected |0> boundary. Since _rN is not referenced elsewhere, summing over it is equivalent to discarding the measurement result. This implements the trace-out semantics without grounds.

Related issue

tqec/tqec#528

Checklist

  • I have added/updated tests (for bugfixes, please include a regression test, for new features, include new tests either to an existing test file or a new file).
  • For new features: I have updated the documentation.
  • All the functions I added have a docstring parsable by sphinx (for a good example see pyzx.extract.extract_circuit).
  • I have added an entry to CHANGELOG.md describing my change.

Copilot AI review requested due to automatic review settings March 30, 2026 07:48
@dlyongemallo dlyongemallo marked this pull request as draft March 30, 2026 07:48
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Fixes an issue in PyZX’s copy rewrite matching that affected hybrid (grounded) graphs produced by circuits with mid-circuit reset, preventing copy_simp/full_reduce from incorrectly rewriting away measurement outcome phases.

Changes:

  • Updated check_copy to explicitly exclude ground vertices from matching the copy rule.
  • Added a regression test based on a Steane X-stabiliser measurement circuit with mid-circuit resets to ensure symbolic measurement phases survive full_reduce.
  • Documented the fix in the changelog.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
pyzx/rewrite_rules/copy_rule.py Prevents check_copy from matching degree-1 ground spiders created by reset/discard semantics.
tests/test_simplify.py Adds regression coverage ensuring ground nodes don’t match check_copy and that full_reduce preserves measurement symbolic phases.
CHANGELOG.md Notes the bugfix in the Unreleased “Fixed” section.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@jvdwetering
Copy link
Copy Markdown
Collaborator

It would be safe to disallow this copying, but it is semantically sound to copy through a ground, as long as the ground copies to both spiders that are produced.
In particular, you mention the 'bug' that the symbolic measurement outcome gets eaten by the ground. That is correct behaviour as measuring something and then discarding should be the same thing as not using that outcome. It sounds like in your use case you don't want the ground to be there at all, since you are not discarding the outcome.

@dlyongemallo
Copy link
Copy Markdown
Contributor Author

The problem I'm trying to understand is the cause of the difference in the post-reduction graphs in tqec/tqec#528 (comment). The c[0] and c[1] vanish after full_reduce.

You're right, this (excluding ground nodes from copy check) is the wrong approach. But I'm still not quite understanding what the right fix would be. I'm now thinking, instead, maybe in graphparser.py where it builds the graph, the symbolic phase c[0] should not have been put on the qubit wire but shoud be branched off into a separate, classical outcome wire, before the ground is attached.

@dlyongemallo dlyongemallo changed the title exclude ground nodes from copy rule separate measurement outcome from qubit wire Mar 30, 2026
@jvdwetering
Copy link
Copy Markdown
Collaborator

I think there is more a problem about mixing two different approaches to representing mixed quantum-classical processes in ZX-diagrams. We can either use grounds to make a distinction between classical and quantum information, or we can not use grounds at all but use parameters to represent measurement outcomes. Not using the parameter for the measurement outcome is then the same as discarding, because then we would have to sum over the two outcomes, which is semantically equal to discarding.

@dlyongemallo
Copy link
Copy Markdown
Contributor Author

@jvdwetering IIUC the problem is that the library has two approaches to representing ground and it would be better to stick to just one. Following up on the conversation in #402 (comment), would it make sense to just remove to_graph_ground, and just always use the symbolic boolean representation for measurements? Is there any reason to keep to_graph_ground if no one is actually using it, and it's causing confusion/bugs?

@dlyongemallo dlyongemallo force-pushed the check_copy_steane_circuit branch from f27e012 to ccf5521 Compare April 4, 2026 06:05
@dlyongemallo
Copy link
Copy Markdown
Contributor Author

I think the to_graph_ground should be removed, assuming it's not being used by anyone. Leaving it around is very confusing and will likely result in misunderstanding and bugs. Please see PR #433.

@jvdwetering
Copy link
Copy Markdown
Collaborator

I agree its confusing. There are some benefits to still having the grounds, as they have special rewrites (a ground eats Hadamards, phases and if you have two grounds after a CNOT, the CNOT gets removed), but in principle we could mimick this behaviour when we have parameters by labelling certain parameters as being 'summed over', which effectively make them the same as grounds.

@dlyongemallo dlyongemallo changed the title separate measurement outcome from qubit wire replace ground-based Reset with symbolic boolean paradigm Apr 27, 2026
@dlyongemallo dlyongemallo force-pushed the check_copy_steane_circuit branch from ccf5521 to 5b7d866 Compare April 27, 2026 06:50
@dlyongemallo dlyongemallo requested a review from Copilot April 27, 2026 06:50
@dlyongemallo
Copy link
Copy Markdown
Contributor Author

I've carried out the suggested "summed-over" change for Reset.

The DiscardBit operation is the lone surviving ground use, on the classical bit wire.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

pyzx/circuit/graphparser.py:195

  • graph_to_circuit now calls _poly_phase_to_conditional_gate(phase, t, ...) for any vertex with a Poly phase (including VertexType.X), but _poly_phase_to_conditional_gate still raises ValueError for non-Z vertex types. This will crash graph extraction for conditional X/NOT gates (which are represented as X spiders with boolean Poly phases). Update _poly_phase_to_conditional_gate to handle VertexType.X (mapping phases to X-rotations/NOT analogously to the Z case), or restore the previous guard so X-type Poly phases fall back to emitting an XPhase gate instead of raising.
                # Conditional gate (both Z and X types are unambiguous
                # now that measurement outcomes are leaves).
                cgate = _poly_phase_to_conditional_gate(phase, t, int(q))
                if cgate is not None:
                    c.add_gate(cgate)
                elif phase != 0:
                    gate_name = "ZPhase" if t == VertexType.Z else "XPhase"
                    c.add_gate(gate_name, q, phase=phase)

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@dlyongemallo dlyongemallo marked this pull request as ready for review April 27, 2026 06:57
@dlyongemallo dlyongemallo marked this pull request as draft April 27, 2026 07:00
@dlyongemallo dlyongemallo force-pushed the check_copy_steane_circuit branch from 5b7d866 to 7fb0126 Compare April 27, 2026 07:09
@dlyongemallo dlyongemallo requested a review from Copilot April 27, 2026 07:10
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread tests/test_simplify.py Outdated
Comment thread pyzx/circuit/graphparser.py Outdated
@dlyongemallo dlyongemallo marked this pull request as ready for review April 27, 2026 08:29
@dlyongemallo dlyongemallo force-pushed the check_copy_steane_circuit branch 3 times, most recently from 51b42f6 to 53ce3c8 Compare April 28, 2026 12:39
@jvdwetering
Copy link
Copy Markdown
Collaborator

I still don't really understand what is going on here and why it is needed.
Is it just that the Reset command is like doing a destructive Measurement followed by |0> state preparation, and so we want this to look exactly like this? Except that a Measurement produces a Boolean variable we care about, while a Reset produces a Boolean variable we do not care about?

I don't understand what all the logic is about creating new boundary spiders. A Reset should not have anything to do with boundary wires.

@dlyongemallo
Copy link
Copy Markdown
Contributor Author

dlyongemallo commented Apr 29, 2026

I still don't really understand what is going on here and why it is needed.

Let's start with the "why it is needed": currently, mid-circuit measure -> reset -> measure loses the symbolic measurement outcome phases when full_reduce is run on the circuit. The added tests test_measurement_outcomes_survive_reduction and test_measurement_outcomes_survive_single_qubit document the problem (these are derived from tqec/tqec#528).

The existing Measurement.to_graph_symbolic_boolean puts a single X(symbol) spider on the qubit wire. This simultaneously represented the measurement and its outcome. The existing Reset was a Z-ground discard, followed by a free-floating X(0) prep. The problem is that full_reduce annihilates the symbolic phases.

As for what's going on: this change moves the symbolic phase to a leaf hanging off the wire. The consequence is that full_reduce cannot propagate or cancel it through wire-level rewrites.

The boundary spider is not identifying the reset. It's just how the "|0> preparation" half of the reset is represented, replacing the old free-floating X(0) prep. This allows for round-trip disambiguation: before, graph_to_circuit couldn't tell a reset apart from a manually disconnected X(0) on an input qubit; with this change, a boundary vertex on a qubit which already has an input is unambiguously a mid-circuit reset. This means we can also recover and round-trip conditional X rotations (NOT, XPhase, SX) which we couldn't before. I believe this fixes the problems identified by tqec#528, but I am still working on that.

The downside is that the disconnected boundary vertex is not in g.inputs()/g.outputs() and tensorfy_native rejects any non-ZXH vertex which isn't registered in those, so g.to_tensor() raises on circuits containing Reset. I'm trying to figure out what to do about that.

@dlyongemallo dlyongemallo marked this pull request as draft April 29, 2026 08:58
@dlyongemallo
Copy link
Copy Markdown
Contributor Author

I'm going to rework this to tag the reset using the vdata instead. That would be more consistent with how measurement is currently implemented as well.

@dlyongemallo dlyongemallo force-pushed the check_copy_steane_circuit branch 7 times, most recently from 69552a6 to ae9769d Compare May 3, 2026 11:36
@dlyongemallo
Copy link
Copy Markdown
Contributor Author

This is now in a state which satisfies the requirements of tqec/tqec#528.

This is a fairly large change. If necessary, I could try to break it up into 3 or 4 smaller PRs.

@dlyongemallo dlyongemallo marked this pull request as ready for review May 3, 2026 11:44
@dlyongemallo dlyongemallo force-pushed the check_copy_steane_circuit branch from ae9769d to 2743dbc Compare May 3, 2026 12:43
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.

3 participants