Added Dynamical QFT + Measurement#807
Added Dynamical QFT + Measurement#807morrishuismans wants to merge 15 commits intomunich-quantum-toolkit:mainfrom
Conversation
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
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:
📝 WalkthroughWalkthroughA new dynamical quantum circuit benchmark module implementing Quantum Fourier Transform with mid-circuit measurements and classical feedback is introduced, replacing traditional O(n²) two-qubit gate implementations. The changelog and build configuration are updated accordingly. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
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: 3
🤖 Fix all issues with AI agents
In `@src/mqt/bench/benchmarks/dynamical_qft.py`:
- Line 21: The description string passed to the register_benchmark decorator for
"dynamical_qft" contains a leading space; update the decorator usage (the
`@register_benchmark` call for "dynamical_qft") to remove the leading space so the
description reads "Dynamic Quantum Fourier Transformation and Measurement (DQFT
+ M)" instead of " Dynamic...".
In `@tests/test_bench.py`:
- Around line 227-234: Add a circuit name assertion to test_dynamical_qft to
match other tests: after creating qc via create_circuit("dynamical_qft", 5)
assert that qc.name (or the circuit's name property used elsewhere) equals
"dynamical_qft" to ensure consistency with tests like test_bv and
test_graphstate_seed; update the test_dynamical_qft function to include this
assertion immediately after the circuit is created.
- Around line 236-245: The test_dynamical_qft_gates currently only checks
Hadamard and measurement counts; add an assertion to verify the conditional
phase rotations by counting entries in qc.data where inst.operation.name is "p"
or the phase gate identifier used by create_circuit for dynamical_qft, and
assert that for num_qubits=5 the phase/controlled-phase gate count equals 10
(4+3+2+1+0). Locate the test function test_dynamical_qft_gates and use qc.data
iteration (same pattern as hadamard_count) to compute phase_count and add the
assert phase_count == 10 so the dynamical QFT's conditional rotations are
covered.
| from ._registry import register_benchmark | ||
|
|
||
|
|
||
| @register_benchmark("dynamical_qft", description=" Dynamic Quantum Fourier Transformation and Measurement (DQFT + M)") |
There was a problem hiding this comment.
Minor: Remove leading space in description.
The description has a leading space " Dynamic..." which appears unintentional.
Proposed fix
-@register_benchmark("dynamical_qft", description=" Dynamic Quantum Fourier Transformation and Measurement (DQFT + M)")
+@register_benchmark("dynamical_qft", description="Dynamic Quantum Fourier Transformation and Measurement (DQFT + M)")🤖 Prompt for AI Agents
In `@src/mqt/bench/benchmarks/dynamical_qft.py` at line 21, The description string
passed to the register_benchmark decorator for "dynamical_qft" contains a
leading space; update the decorator usage (the `@register_benchmark` call for
"dynamical_qft") to remove the leading space so the description reads "Dynamic
Quantum Fourier Transformation and Measurement (DQFT + M)" instead of "
Dynamic...".
| def test_dynamical_qft() -> None: | ||
| """Test the creation of the Dynamical Quantum Fourier Transform + Measurement.""" | ||
| qc = create_circuit("dynamical_qft", 5) | ||
|
|
||
| # Check Qubit number and Classical register number | ||
| assert qc.num_qubits == 5 | ||
| assert qc.num_clbits == 5 | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding circuit name assertion for consistency.
Other similar tests (e.g., test_bv, test_graphstate_seed) also verify the circuit name. Consider adding an assertion for completeness:
Optional enhancement
def test_dynamical_qft() -> None:
"""Test the creation of the Dynamical Quantum Fourier Transform + Measurement."""
qc = create_circuit("dynamical_qft", 5)
# Check Qubit number and Classical register number
assert qc.num_qubits == 5
assert qc.num_clbits == 5
+ assert qc.name == "dynamical_qft"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_dynamical_qft() -> None: | |
| """Test the creation of the Dynamical Quantum Fourier Transform + Measurement.""" | |
| qc = create_circuit("dynamical_qft", 5) | |
| # Check Qubit number and Classical register number | |
| assert qc.num_qubits == 5 | |
| assert qc.num_clbits == 5 | |
| def test_dynamical_qft() -> None: | |
| """Test the creation of the Dynamical Quantum Fourier Transform + Measurement.""" | |
| qc = create_circuit("dynamical_qft", 5) | |
| # Check Qubit number and Classical register number | |
| assert qc.num_qubits == 5 | |
| assert qc.num_clbits == 5 | |
| assert qc.name == "dynamical_qft" |
🤖 Prompt for AI Agents
In `@tests/test_bench.py` around lines 227 - 234, Add a circuit name assertion to
test_dynamical_qft to match other tests: after creating qc via
create_circuit("dynamical_qft", 5) assert that qc.name (or the circuit's name
property used elsewhere) equals "dynamical_qft" to ensure consistency with tests
like test_bv and test_graphstate_seed; update the test_dynamical_qft function to
include this assertion immediately after the circuit is created.
tests/test_bench.py
Outdated
| def test_dynamical_qft_gates() -> None: | ||
| """Test the gates used in the Dynamical Quantum Fourier Transform + Measurement.""" | ||
| qc = create_circuit("dynamical_qft", 5) | ||
|
|
||
| # Check the number of Hadamard gates and Measurements | ||
| hadamard_count = sum(1 for inst in qc.data if inst.operation.name == "h") | ||
| measurement_count = sum(1 for inst in qc.data if inst.operation.name == "measure") | ||
| assert hadamard_count == 5 | ||
| assert measurement_count == 5 | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding test coverage for conditional phase rotations.
The test verifies Hadamard and measurement counts, but the distinguishing feature of the dynamic QFT is the conditional phase rotations. Consider adding verification for the phase gate count or structure.
For num_qubits=5, the expected number of phase gates is 4+3+2+1+0 = 10.
Suggested enhancement
def test_dynamical_qft_gates() -> None:
"""Test the gates used in the Dynamical Quantum Fourier Transform + Measurement."""
qc = create_circuit("dynamical_qft", 5)
# Check the number of Hadamard gates and Measurements
hadamard_count = sum(1 for inst in qc.data if inst.operation.name == "h")
measurement_count = sum(1 for inst in qc.data if inst.operation.name == "measure")
assert hadamard_count == 5
assert measurement_count == 5
+
+ # Verify conditional blocks exist (if_else instructions contain the phase rotations)
+ if_else_count = sum(1 for inst in qc.data if inst.operation.name == "if_else")
+ assert if_else_count == 5 # One conditional block per qubit📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_dynamical_qft_gates() -> None: | |
| """Test the gates used in the Dynamical Quantum Fourier Transform + Measurement.""" | |
| qc = create_circuit("dynamical_qft", 5) | |
| # Check the number of Hadamard gates and Measurements | |
| hadamard_count = sum(1 for inst in qc.data if inst.operation.name == "h") | |
| measurement_count = sum(1 for inst in qc.data if inst.operation.name == "measure") | |
| assert hadamard_count == 5 | |
| assert measurement_count == 5 | |
| def test_dynamical_qft_gates() -> None: | |
| """Test the gates used in the Dynamical Quantum Fourier Transform + Measurement.""" | |
| qc = create_circuit("dynamical_qft", 5) | |
| # Check the number of Hadamard gates and Measurements | |
| hadamard_count = sum(1 for inst in qc.data if inst.operation.name == "h") | |
| measurement_count = sum(1 for inst in qc.data if inst.operation.name == "measure") | |
| assert hadamard_count == 5 | |
| assert measurement_count == 5 | |
| # Verify conditional blocks exist (if_else instructions contain the phase rotations) | |
| if_else_count = sum(1 for inst in qc.data if inst.operation.name == "if_else") | |
| assert if_else_count == 5 # One conditional block per qubit |
🤖 Prompt for AI Agents
In `@tests/test_bench.py` around lines 236 - 245, The test_dynamical_qft_gates
currently only checks Hadamard and measurement counts; add an assertion to
verify the conditional phase rotations by counting entries in qc.data where
inst.operation.name is "p" or the phase gate identifier used by create_circuit
for dynamical_qft, and assert that for num_qubits=5 the phase/controlled-phase
gate count equals 10 (4+3+2+1+0). Locate the test function
test_dynamical_qft_gates and use qc.data iteration (same pattern as
hadamard_count) to compute phase_count and add the assert phase_count == 10 so
the dynamical QFT's conditional rotations are covered.
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: 1
🤖 Fix all issues with AI agents
In `@pyproject.toml`:
- Around line 22-24: The pyproject currently pins "qiskit[qasm3-import]>=2.0.0"
while the comment references fixes present in Qiskit 1.3.2; either lower the
minimum to the appropriate older version (e.g., "qiskit[qasm3-import]>=1.3.2" or
the exact earliest release containing the OpenQASM3 export fixes) or add a clear
justification next to the dependency line explaining why 2.0.0 is required
(mentioning the specific features like BoxOp or control-flow changes if relied
upon); update the dependency string "qiskit[qasm3-import]>=2.0.0" or the
adjacent comment accordingly.
|
@flowerthrower @burgholzer Dear Lukas and Patrick, could you please review my code? Thanks in advance! |
Signed-off-by: morrishuismans <97433985+morrishuismans@users.noreply.github.com>
burgholzer
added
python
burgholzer
left a comment
burgholzer
left a comment
There was a problem hiding this comment.
Thanks for your work on this!
This looks like great progress already.
You can find some suggestions/feedback in the comments below.
I hope none of them should really be hard to address.
You will also need to merge in the latest changes from main so that the conflicts are resolved.
|
|
||
| @register_benchmark("dynamical_qft", description="Dynamic Quantum Fourier Transformation and Measurement (DQFT + M)") | ||
| def create_circuit(num_qubits: int) -> QuantumCircuit: | ||
| """Returns a quantum circuit implementing the Dynamic Quantum Fourier Transform and Measurement algorithm. |
There was a problem hiding this comment.
This docstring could be made a little more detailed and explain a little bit more about the algorithm itself. From this docstring alone, no one would have an idea of what the algorithm is doing. There is also no reference to some kind of resource this is based on.
| Arguments: | ||
| num_qubits: number of qubits of the returned quantum circuit | ||
|
|
||
| Returns: | ||
| QuantumCircuit: a quantum circuit implementing the Dynamic Quantum Fourier Transform and Measurement algorithm | ||
| """ | ||
| q = QuantumRegister(num_qubits, "q") | ||
| cl = ClassicalRegister(num_qubits, "cl") | ||
| qc = QuantumCircuit(q, cl, name="dynamical_qft") | ||
| for i in range(num_qubits): | ||
| qc.h(q[i]) | ||
| qc.measure(q[i], cl[i]) | ||
| with qc.if_test((cl[i], 1)): | ||
| for j in range(1, num_qubits - i): | ||
| qc.p(2 * np.pi / 2 ** (j + 1), q[j + i]) | ||
| return qc |
There was a problem hiding this comment.
Initially I was thinking this isn't really the way I imagined this to be written, because I would have imagined this to be a single-qubit circuit that repeatedly resets the qubit and applies phase corrections to it based on previous measurements (similar to iQPE from #806).
However, after thinking about it a little more, this kind of does make sense to portray as an n qubit circuit. This also allows using this circuit as a benchmark for circuit optimization techniques that would, e.g., introduce qubit reuse to shrink the circuit.
There was a problem hiding this comment.
Micro optimization: The phase computation can be simplified a bit here
qc.p(2 * np.pi / 2 ** (j + 1), q[j + I])
is equivalent to
qc.p(np.pi / (2 ** j), q[j + I])
There was a problem hiding this comment.
Slight naming tweak: The canonical name for classical registers in Qiskit is c (not cl) might be worth adapting that.
There was a problem hiding this comment.
Similar to #806, this will need an upper bound on the number of qubits since the shift operation only makes sense up to a certain number of qubits and is not well defined afterwards.
There was a problem hiding this comment.
Similar to #806, this will need an upper bound on the number of qubits since the shift operation only makes sense up to a certain number of qubits and is not well defined afterwards.
Dear @burgholzer, I am not really sure of what you mean. Do you mean that the Phase gates with the very small phases are unnecessary? If so, would it be better to upper bound the number of qubits or the number of phase gates per layer and discard all gates with negligible phase?
Or do you mean something different with the shift operation?
There was a problem hiding this comment.
This is computing 2**j, which is only well defined up until j<=63 (or less; needs to be checked) due to integer overflow.
Two solutions basically:
- do not permit generating larger instances
- add an additional check that approximates angles and drops gates if the angle becomes too small
|
Dear @morrishuismans, may I ask for the current status of this PR? |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@CHANGELOG.md`:
- Line 15: The changelog entry references PR [`#807`] but the PR link list is
missing that reference; add a link entry for PR `#807` to the existing PR link
list so the bracketed reference resolves (use the same link format as other
entries, e.g., " [`#807`]: <PR URL>" pointing to the repository pull/807), and
ensure you update all occurrences of the changelog where the missing link
affects rendering (the same section around the Dynamical QFT + Measurement
circuit entry).
CHANGELOG.md
Outdated
| ### Added | ||
|
|
||
| - ✨ Add support for compiling `if_test` for all gatesets and devices ([#805]) ([**@adnathanail**]) | ||
| - ✨ Add Dynamical QFT + Measurement circuit as a dynamical benchmarking circuit ([#807]) ([**@morrishuismans**]) |
There was a problem hiding this comment.
Add the missing PR link for [#807].
The new changelog entry references [#807], but the PR link list doesn’t include it, so the link will render as unresolved.
🔧 Proposed fix
<!-- PR links -->
[`#805`]: https://github.com/munich-quantum-toolkit/bench/pull/805
+[`#807`]: https://github.com/munich-quantum-toolkit/bench/pull/807
[`#803`]: https://github.com/munich-quantum-toolkit/bench/pull/803Also applies to: 104-140
🤖 Prompt for AI Agents
In `@CHANGELOG.md` at line 15, The changelog entry references PR [`#807`] but the PR
link list is missing that reference; add a link entry for PR `#807` to the
existing PR link list so the bracketed reference resolves (use the same link
format as other entries, e.g., " [`#807`]: <PR URL>" pointing to the repository
pull/807), and ensure you update all occurrences of the changelog where the
missing link affects rendering (the same section around the Dynamical QFT +
Measurement circuit entry).
3 participants
Description
Adds the Dynamical QFT + Measurement circuit as a dynamical benchmarking circuit.
Fixes #798
Checklist: