Added GHZ dynamic implementation and its tests#816
Added GHZ dynamic implementation and its tests#816burgholzer merged 27 commits intomunich-quantum-toolkit:mainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new dynamic GHZ benchmark module that builds GHZ circuits using intermediate measurements and conditional gates. Introduces a registered Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/mqt/bench/benchmarks/ghz_dynamic.py`:
- Around line 45-50: The conditional reset uses the wrong classical bit (c[0])
for every odd-qubit measurement; update the if_test to check the classical bit
corresponding to the measurement just performed. In the loop that calls
qc.measure(i, j) and then increments j, replace with qc.if_test((c[j-1], 1))
(i.e., reference the bit index of the measurement just stored) so each odd-qubit
reset is gated by its own measurement result; modify the qc.if_test call in the
loop accordingly.
- Around line 26-28: The ClassicalRegister created as
ClassicalRegister(num_qubits // 2) can be zero-sized when num_qubits == 1 which
Qiskit rejects; update the ghz_dynamic circuit construction to avoid creating a
zero-length classical register: detect when num_qubits == 1 and in that case
omit creating the mid_measurement ClassicalRegister and construct QuantumCircuit
with only the QuantumRegister, otherwise create the mid_measurement register as
now; alternatively (if you prefer stricter validation) add a benchmark-level
guard in the input validation for ghz_dynamic to reject num_qubits < 2 and
update tests accordingly—use the names QuantumRegister, ClassicalRegister and
the qc/ghz_dynamic circuit construction to locate the code to change.
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@src/mqt/bench/benchmarks/ghz_dynamic.py`:
- Line 30: Replace the typo in the inline comment "Apply Haddamard gates to all
even qubits" with the correct spelling "Hadamard" so the comment reads "Apply
Hadamard gates to all even qubits"; locate this exact comment string in the file
and update it accordingly.
- Around line 56-62: Fix the typo in the comment ("exor" → "XOR") and update the
bit comparison to use an integer bit value: replace expr.equal(condition, True)
with expr.equal(condition, 1) in the loop that applies X gates (referencing
variables/function names: condition, c, qc.if_test, qc.x in ghz_dynamic.py) so
the classical bit comparison uses 1 instead of the Python boolean.
- Around line 19-69: The ghz_dynamic benchmark contains mid-circuit measurements
which Solovay-Kitaev (used by the "clifford+t" gateset) cannot handle; update
the test_quantumcircuit_levels test to skip/exclude the "ghz_dynamic" benchmark
from gateset iteration that uses Solovay-Kitaev (same pattern used for "shor"
exclusion) so that benchmarks with mid-circuit measurements are not passed to
SolovayKitaev-based transpilation.
In `@tests/test_bench.py`:
- Around line 238-244: The test accesses mid_meas_regs[0] and
measure_all_regs[0] without checking that the filtered lists from qc.cregs are
non-empty; update the assertions to first assert that mid_meas_regs and
measure_all_regs are not empty (e.g., assert mid_meas_regs, "no mid_measurement
register found in qc.cregs") and assert measure_all_regs similarly, then check
sizes against num_qubits // 2 and num_qubits respectively so failures produce
clear messages referencing mid_meas_regs, measure_all_regs, qc.cregs, and
num_qubits.
- Around line 227-231: The test includes num_qubits=1 which triggers an upstream
bug in ghz_dynamic.create_circuit (creating ClassicalRegister(0)); either fix
ghz_dynamic.create_circuit to handle the single-qubit case or (simpler) remove 1
from the pytest.mark.parametrize list so tests only use valid sizes (e.g.,
[2,3,7,10]); also add an explicit type annotation to the test function signature
(num_qubits: int) so the parameter is typed. Ensure references to
ghz_dynamic.create_circuit remain unchanged when updating the test.
…v transpiler for mid-circuit measurements
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@CHANGELOG.md`:
- Line 15: The CHANGELOG entry references [`#816`] but the PR link definition is
missing; add a link definition for [`#816`] in the PR links section (the
footnote/link reference area of CHANGELOG.md) so the reference resolves
correctly, e.g. adding a line like "[`#816`]:
https://github.com/<org>/<repo>/pull/816" (replace <org>/<repo> with the
repository owner/name) adjacent to the other PR link definitions to match
formatting used for other entries.
In `@tests/test_bench.py`:
- Line 230: Update the docstring that currently reads "Verify the structure od
the dynamic GHZ circuit for various qubit counts." to correct the typo by
replacing "od" with "of" so it reads "Verify the structure of the dynamic GHZ
circuit for various qubit counts."; locate the exact docstring text in
tests/test_bench.py and apply the single-word correction.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/test_bench.py (1)
105-116:ghz_dynamicmust be excluded from gateset iteration to prevent SolovayKitaev failures.The
test_quantumcircuit_levelstest iterates over all benchmarks and all gatesets, includingclifford+twhich uses Solovay-Kitaev decomposition. Sinceghz_dynamiccontains mid-circuit measurements (non-unitary operations), it cannot be transpiled with this gateset and will cause the test to fail.🐛 Proposed fix
- if benchmark_name != "shor": + if benchmark_name not in {"shor", "ghz_dynamic"}: for gateset_name in get_available_gateset_names():
🤖 Fix all issues with AI agents
In `@CHANGELOG.md`:
- Line 15: The changelog references the contributor marker [**@TomasVF**] but
there is no corresponding contributor link definition in the Contributors
section; add a new contributor link definition for `@TomasVF` in the Contributor
section (after the existing entries) using the same link format as other
contributors so the author link renders correctly, ensuring the exact identifier
"@TomasVF" is used to match the changelog entry.
In `@src/mqt/bench/benchmarks/ghz_dynamic.py`:
- Around line 34-50: Replace the reused, ambiguous variable names in
ghz_dynamic.py with descriptive ones: in the CNOT loop rename j and z to
prev_qubit and next_qubit (use prev_qubit = i - 1 and next_qubit = i + 1 and
update qc.cx calls), in the measurement loop initialize and use clbit_idx (set
clbit_idx = 0 before the loop, use qc.measure(i, clbit_idx), with
qc.if_test((c[clbit_idx], 1)) and then clbit_idx += 1), and similarly rename any
XOR-loop counter to xor_idx; ensure all references to j/z in qc.cx, qc.measure,
and with qc.if_test are updated to the new names and that semantics remain
unchanged (keep control qubit as i and bounds checks using num_qubits).
|
@burgholzer @flowerthrower Dear Lukas and Patrick all the important errors from the rabbit ai are allready solved and the PR is ready for the review. |
burgholzer
added
python
burgholzer
left a comment
burgholzer
left a comment
There was a problem hiding this comment.
Thanks for the contribution here! This is quite an interesting use case of dynamic circuits that I haven't seen before.
You can find some feedback on the code in the comments below. I believe this will need one more round of cleanup until it is ready to be merged. That should, hopefully, not take too long though.
You'll also need to merge in the main branch to resolve the conflicts 😌
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/mqt/bench/benchmarks/ghz_dynamic.py`:
- Around line 21-22: Fix the typos in the docstring in ghz_dynamic.py: change
"dependant" to "dependent" and "clasical" to "classical" in the module/function
docstring that describes the dynamic GHZ circuit (the multiline string starting
"Returns a dynamic quantum circuit implementing the GHZ state...").
- Line 53: Rename the misspelled variable clasical_register to
classical_register everywhere it's used (the declaration and all references
within the GHZ benchmark code) to correct readability and prevent confusion;
update the variable name in the declaration and in any assignments,
conditionals, or function calls that reference clasical_register so all usages
consistently use classical_register.
- Around line 62-63: The guard "if num_qubits > 1" is redundant because the
num_qubits == 1 case already returns early; remove the conditional and inline
its body directly after "j = 0" so the code that was wrapped by "if num_qubits >
1" executes unconditionally for the remaining flow; update any indentation and
references to variable j accordingly (look for the "j = 0" assignment and the
following "if num_qubits > 1" block in ghz_dynamic.py).
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/mqt/bench/benchmarks/ghz_dynamic.py`:
- Around line 62-76: The variable j is reused for two unrelated purposes which
harms readability; rename the XOR counter used with mid_measure/condition in the
first loop to a descriptive name (e.g., xor_idx or xor_count) and update its
uses in that loop (initialization, increment, and when computing condition =
expr.bit_xor(condition, mid_measure[xor_idx])), and keep the second loop's j as
the next-qubit index (or rename it to next_q) used in qc.cx(i, j) so the two
loops use distinct, descriptive variable names (reference symbols: mid_measure,
condition, num_qubits, qc.x in the first loop and qc.cx in the second loop).
Signed-off-by: Tomás Villalba Ferreiro <tomas@terrae.net>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/mqt/bench/benchmarks/ghz_dynamic.py`:
- Around line 20-26: The docstring for create_circuit is a single long line and
should be reformatted into a multi-line triple-quoted docstring: start with a
short one-line summary, then a blank line, then a longer description wrapped to
reasonable line lengths (e.g., ~80 chars) that includes the note about dynamic
GHZ construction and the arXiv link, and finish with an "Arguments:" section
documenting num_qubits and its type; update the docstring for the create_circuit
function to this standard multi-line format to improve readability.
- Line 73: Remove the redundant initial assignment of next_qubit (`next_qubit =
0`) which is immediately overwritten later in the loop; delete that line and, if
desired, inline the assignment expression used at the later `next_qubit = ...`
site so only the necessary assignment remains (look for `next_qubit` in the loop
in ghz_dynamic.py and remove the initial initialization before the overwritten
assignment).
|
Dear @TomasVF, may I ask for the current status of this PR? |
There were some corrections i had to do but i think everything is fixed now. I'm waiting for |
I am probably missing something, but I don't see any answers from you. Could it be that these are still unpublished on your side? |
My mistake, I completly forgot to publish the review. It should already be visible now. |
Signed-off-by: burgholzer <burgholzer@me.com>
Signed-off-by: burgholzer <burgholzer@me.com>
burgholzer
left a comment
burgholzer
left a comment
There was a problem hiding this comment.
Thanks @TomasVF for the iteration. I pushed a couple of updates to the branch that slightly polish the code. This should be good to go now.
Thanks for the nice contribution.
3 participants
Description
Please include a summary of the change and, if applicable, which issue is fixed.
Please also include relevant motivation and context.
List any dependencies that are required for this change.
Fixes #815
Checklist: