Optimize statevector simulator preprocessing#316
Open
Kenny-Heitritter wants to merge 33 commits into
Open
Conversation
…g, OpenMP support Key changes: - Remove unroll() from Simulator.run() for QasmModule input (caller responsibility) - Single-pass _preprocess() with inline gate fusion (replaces separate _fuse_gates pass) - Native gate handling in _preprocess() without requiring full AST unrolling - AST expression evaluator for raw QASM parameter expressions (pi, tau, arithmetic) - Cache unroll() results in QasmModule (skip if already unrolled) - New Cython apply_circuit() entry point with 5 specialized kernels - Optional OpenMP parallelism via PYQASM_NUM_THREADS env var (default=1/serial) - OpenMP build detection in setup.py (macOS Homebrew libomp + Linux) - Benchmark suite comparing PyQASM vs Qiskit Aer, Cirq, PennyLane Lightning - Performance regression tests with pytest benchmark marker - Profiling script for detailed per-phase timing breakdown Performance (serial, median of 5): - Random circuits: PyQASM fastest at all sizes (4-22q), beating Qiskit/Cirq/Lightning - QFT circuits: PyQASM fastest at ≤12q; kernel-bound at ≥16q (memory bandwidth limited) - With PYQASM_NUM_THREADS=8: 4.1x speedup at 22q, 2.7x at 20q Known limitation: macOS libomp conflicts with Qiskit Aer's OpenMP in same process, so parallelism is opt-in. Future work: SIMD/AVX kernels for single-threaded perf. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Port the non-cache Run 06 AlphaEvolve statevector improvements into the sv branch implementation while deliberately excluding the QASM-string instruction memoization that overfit the AE repeated-timing harness. The simulator still preprocesses every input on each run call. The speedup comes from reducing Python overhead in that preprocessing path: - add optional numba-jitted helpers for small rotation/diagonal utilities - hoist matrix constants and phase constants to module scope - keep pending diagonal one-qubit gates as phase pairs during fusion - flush diagonal gates directly to the diagonal Cython kernel - preallocate packed instruction arrays instead of growing Python lists - decompose SWAP through the optimized controlled-X kernel path - preserve diagonal pending gates across CZ/CRZ because they commute - keep the conservative capacity bound needed for swap expansion and pending-gate flushes This is the honest non-cache improvement: no _PROGRAM_INSTRUCTION_CACHE, no cross-call memoization, and no reliance on repeated identical QASM strings. Add a simulation extra for installing numba; the code falls back to pure Python helpers when numba is absent.
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds a NumPy/Cython statevector simulator with OpenMP-capable kernels, QASM preprocessing and gate fusion, public simulator APIs, correctness and performance tests, benchmark/profiling scripts, packaging changes, documentation, and CI workflow updates. ChangesStatevector Simulator
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant Simulator
participant Preprocess as _preprocess
participant Kernel as sv_sim.apply_circuit
participant RNG as numpy.random.Generator
Caller->>Simulator: run(QASM or QasmModule, shots)
Simulator->>Preprocess: compile unrolled circuit
Preprocess-->>Simulator: packed instructions
Simulator->>Kernel: apply instructions to statevector
Kernel-->>Simulator: final statevector
Simulator->>RNG: sample measurement outcomes
Simulator-->>Caller: SimulatorResult
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
Kenny-Heitritter
marked this pull request as draft
May 26, 2026 14:44
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@benchmarks/bench_simulator.py`:
- Around line 344-348: The equality checks in the checks dict compare raw
statevectors (sv_pyqasm vs sv_qiskit, sv_cirq_le, sv_pl_le) and will fail when
candidates differ only by a global phase; normalize each candidate to
sv_pyqasm's global phase before calling np.allclose. For each candidate vector
(sv_qiskit, sv_cirq_le, sv_pl_le) compute a phase factor against sv_pyqasm
(e.g., using the normalized inner product np.vdot(sv_pyqasm,
sv_candidate)/abs(...), or by matching a nonzero component) and multiply/divide
the candidate by the conjugate of that phase so its global phase matches
sv_pyqasm, then use np.allclose on the aligned vectors when constructing the
checks dict.
In `@setup.py`:
- Around line 13-14: BASE_COMPILE_ARGS and BASE_LINK_ARGS are currently
hardcoded for GCC/Clang and include host-specific ISA flags; update setup.py to
make flags toolchain-aware by removing "-march=native" (and likely
"-ffast-math") from unconditional defaults and set compile/link flags based on
detected compiler type: inspect the build compiler via the distutils/setuptools
compiler instance or compiler.compiler_type and, for 'msvc', use
MSVC-appropriate flags (e.g. '/O2' and '/openmp' when OpenMP is available) and
for 'unix' use '-O3' and '-fopenmp' only when detected; change _check_openmp()
to probe OpenMP support using the project compiler (invoke
compiler.compile/compiler.link methods or use a temporary Extension building
attempt) rather than hardcoding cmd = ["cc", ...], return correct compile and
link flags to apply to extra_compile_args and extra_link_args for each Extension
(refer to BASE_COMPILE_ARGS, BASE_LINK_ARGS, extra_compile_args, and
_check_openmp in the diff).
In `@src/pyqasm/accelerate/sv_sim.pyx`:
- Around line 351-369: In apply_circuit, avoid dereferencing &array[0] on
possibly-empty Cython buffers; first check the lengths (e.g., ensure sv,
gate_params, diag_phases, two_qubit_gates have >0 elements or that
n_instructions>0 where appropriate) and only set sv_ptr, gp_ptr, dp_ptr, tq_ptr
to &array[0] when the buffer is non-empty, otherwise set the pointer to NULL (or
skip pointer usage) to prevent invalid memory access; update uses of
sv_ptr/gp_ptr/dp_ptr/tq_ptr later in the function to handle NULL/empty cases
safely.
- Around line 22-23: sv_sim.pyx currently cimports omp_get_max_threads and
omp_set_num_threads and calls omp_set_num_threads at module import time which
forces an OpenMP dependency even when setup.py disables it; modify the file to
guard all OpenMP cimports and calls behind a compile-time macro (e.g. `#IFDEF`
HAVE_OPENMP) driven by the same check used in setup.py so the Cython module
builds without OpenMP: wrap the cimports omp_get_max_threads/omp_set_num_threads
and any omp_* calls in conditional compilation blocks and provide safe no-op
fallbacks when the macro is not defined; additionally harden apply_circuit by
adding precondition checks (or early returns) for empty/mismatched memoryviews
(the function apply_circuit and its caller _preprocess/statevector code paths)
so it never takes &sv[0], &gate_params[0], &diag_phases[0], or
&two_qubit_gates[0] when the buffers are empty.
In `@tests/test_sv_sim.py`:
- Around line 309-314: The current comparison in tests/test_sv_sim.py only
normalizes global phase when the string "global phase" appears in description,
which is brittle; change the logic in the block that references description,
sv_expected, sv_actual, idx, and phase so that the global-phase normalization is
applied unconditionally (or replace it with a phase-invariant comparator helper)
by removing the conditional check on description and always computing idx =
np.argmax(np.abs(sv_expected) > 1e-10), phase = sv_actual[idx] /
sv_expected[idx], and sv_actual = sv_actual / phase before comparing vectors.
- Around line 44-45: Change the pytest.mark.parametrize argnames from a
comma-separated string to an explicit tuple so it satisfies PT006; locate the
pytest.mark.parametrize call in tests/test_sv_sim.py that currently uses "qasm,
description" and replace the argnames with a tuple form like ("qasm",
"description") (or equivalent single-quoted tuple) while keeping the existing
parameter values and test name intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: a06ded4b-859c-4557-9d06-5128c623731b
⛔ Files ignored due to path filters (2)
benchmarks/bench_qft.pngis excluded by!**/*.pngbenchmarks/bench_random.pngis excluded by!**/*.png
📒 Files selected for processing (12)
.gitignorebenchmarks/bench_simulator.pybenchmarks/plot_benchmarks.pybenchmarks/profile_simulator.pypyproject.tomlsetup.pysrc/pyqasm/accelerate/sv_sim.pyxsrc/pyqasm/modules/base.pysrc/pyqasm/simulator/__init__.pysrc/pyqasm/simulator/statevector.pytests/test_perf_regression.pytests/test_sv_sim.py
Kenny-Heitritter
marked this pull request as ready for review
May 26, 2026 15:05
Two independent CI failures are addressed: 1. Source-distribution job (6 test failures): remove the unroll() caching shortcut in QasmModule that early-returned when the module looked already unrolled. It broke re-unrolling (e.g. unroll(external_gates=...) followed by unroll() to flush them), failing the depth/rebase/include tests. This also contradicted the PR's stated "preprocess on every run" goal. 2. Wheel-build jobs (qiskit unbuildable in manylinux containers): qiskit / qiskit-aer have no prebuilt wheels for the cibuildwheel containers and fall back to a from-source build needing Rust. Move those reference-oracle deps into a dedicated `test-sim` extra so the wheel test env (test,cli,pulse) no longer pulls them, and guard the qiskit imports in test_sv_sim.py with pytest.importorskip so the module skips cleanly where qiskit is absent. The sdist and coverage jobs install `test-sim` to keep running the comparison on the standard runners where qiskit installs fine. Also exclude the hardware-dependent benchmark perf-regression tests from the CI test runs (-m "not benchmark"), per tests/test_perf_regression.py's own docs.
The build appended -O3 -ffast-math -march=native unconditionally to every
Extension, regardless of compiler or architecture. This is wrong for
distributed wheels:
* -march=native ties the wheel to the build machine's CPU and can SIGILL on
older hardware; it is also not the right spelling on MSVC or arm64.
* MSVC silently ignores the GCC/Clang flags, so Windows wheels got none of
the intended optimization.
* -ffast-math relaxes IEEE semantics, which the statevector simulator relies
on.
Replace the static flag list with a build_ext subclass that selects flags per
compiler: -O3 for GCC/Clang, /O2 for MSVC, and OpenMP (-fopenmp / /openmp)
applied only to the sv_sim kernel. No -march (baseline ISA, matching SciPy and
scikit-learn) and no -ffast-math.
The macOS wheel jobs failed in delocate: the sv_sim OpenMP build links Homebrew's libomp, whose dylib has a minimum-macOS target newer than the wheel's macosx_11_0 tag, so delocate refuses to bundle it. (Pre-existing in this PR's OpenMP-on-macOS approach.) Disable OpenMP on macOS by default so no libomp is linked or bundled and the wheels stay portable (sv_sim builds single-threaded on macOS only; Linux and Windows keep OpenMP). A local source build can opt back in with PYQASM_MACOS_OPENMP=1. Drop the now-unnecessary `brew install libomp` step from the wheel workflows.
Disabling the -fopenmp flag on macOS was not enough: sv_sim.pyx unconditionally cimports prange/openmp, so the generated C still pulled in an unguarded <omp.h> and failed to compile on macOS (no system libomp). Gate the OpenMP imports, thread setup, and every prange branch behind a Cython compile-time IF USE_OPENMP. The serial `with nogil` paths (which already existed alongside each prange) become the sole code path when OpenMP is off, and no <omp.h> is emitted. setup.py decides USE_OPENMP once (Linux/Windows on, macOS off unless PYQASM_MACOS_OPENMP=1) and passes it to cythonize via compile_time_env, matching the compiler flags applied in build_ext. Verified: Linux/Windows OpenMP build and the serial macOS build are bit-identical, and the threaded prange path matches the serial path.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
The simulator's crz fast path used a controlled-diagonal instruction that
can only phase the |control=1, target=1> amplitude, so it implemented a
controlled-phase gate diag(1,1,1,e^{i*theta/2}) instead of a true
controlled-Rz diag(1,1,e^{-i*theta/2},e^{i*theta/2}) -- it dropped the
phase on |control=1, target=0>. Route crz through the general
controlled-gate kernel with the full Rz matrix instead.
Add tests/test_sv_sim_core.py: a qiskit-free test module (the existing
simulator tests importorskip qiskit and are skipped in wheel-build
containers, and never exercised crz). It regression-tests crz via a
self-contained differential against pyqasm's own decomposition, plus
known-action gate checks, fusion paths, error handling, and helper unit
tests. Raises statevector.py coverage from 78% to 95%; the remainder is
dead/defensive code (notably the never-called generic two-qubit path).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Member
Follow-up: dead generic two-qubit-gate scaffoldingWhile adding coverage/tests for the statevector simulator I noticed the generic two-qubit path is fully built but never reached, so flagging it here so we don't forget to either wire it up or remove it:
These lines are the bulk of the remaining uncovered code in Context: this is separate from the |
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/main.yml:
- Line 188: The codecov/codecov-action reference in the workflow is using a
version tag (v6.0.0) which is mutable and poses a supply-chain risk. Replace the
version tag with a full commit SHA in the uses statement for
codecov/codecov-action to pin it to an immutable commit, preventing the tag from
being rewritten to malicious code.
In `@bin/cibw/test_wheel.sh`:
- Line 36: The pytest path argument $project/tests in the pytest command is not
quoted, which can cause word splitting and globbing issues if the project path
contains spaces or special characters. Add double quotes around $project/tests
to ensure the path is treated as a single argument, changing it from python -m
pytest $project/tests -m "not benchmark" to properly quote the path variable.
In `@tests/test_sv_sim_core.py`:
- Around line 161-166: The test function `test_single_qubit_gate_action` is
missing a type annotation for its parameter `gate`. Add a type annotation of
`str` to the `gate` parameter since it receives string values from the
parametrize decorator. This will comply with the coding guideline that requires
all functions, methods, and class attributes to have type annotations.
- Around line 60-64: The _embed function is missing both a docstring and type
annotations as required by the coding guidelines. Add type annotations to the
function parameters (mat, target, num_qubits) and the return type to indicate
they are numpy arrays and integers respectively. Additionally, add a
comprehensive docstring at the beginning of the _embed function that explains
its purpose (embedding an operation on a target qubit into a larger Hilbert
space), describes each parameter, and specifies what the function returns.
- Around line 67-78: The function _embed_controlled is missing both type
annotations and a docstring as required by the coding guidelines. Add type
annotations to all parameters (mat, control, target, num_qubits) and to the
return type of the function. Additionally, add a docstring at the beginning of
the function that explains its purpose, describes each parameter with its
expected type and meaning, and describes what the function returns.
- Around line 101-106: The assert_sv_close function is missing type annotations
and has an incomplete docstring. Add type annotations for all parameters
(actual, expected, atol) and the return type to the function signature. Expand
the docstring to document what each parameter represents, what values they
should contain, what the atol parameter controls, and clarify that the function
returns None and performs assertion checks rather than returning a result.
- Around line 123-136: The function test_crz_fast_path_matches_decomposition is
missing type annotations on its parameters. Add type annotations to all function
parameters: theta should be annotated as float, and both control and target
should be annotated as int. This ensures compliance with the coding guideline
requiring all functions to have type annotations.
- Around line 109-112: The `run_sv` function is missing both a docstring and
type annotations as required by coding guidelines. Add type annotations to all
parameters (qasm and external_gates) and the return type to the function
signature. Add a docstring to the function that explains its purpose (what it
does with the qasm string and external gates), describes each parameter and its
expected type, and describes what the function returns (the final statevector).
- Around line 81-98: The reference_statevector function is missing type
annotations and has an incomplete docstring that does not follow Python
conventions. Add type annotations to the function signature for the ops
parameter (a list of tuples containing operation information), the num_qubits
parameter (an integer), and the return type (numpy ndarray). Replace the current
single-line docstring with a properly formatted docstring that includes separate
Args and Returns sections, clearly documenting what each parameter represents
and what the function returns.
- Around line 349-352: The loads_unrolled function is missing both a docstring
and type annotations as required by coding guidelines. Add type annotations for
the qasm parameter and the return type of the loads_unrolled function, and add a
comprehensive docstring that explains the function's purpose, describes the qasm
parameter, and documents the return value (which should be annotated as
QasmModule or the appropriate return type).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e9fd7557-16b1-4b89-be87-9ed5cfe2ff59
⛔ Files ignored due to path filters (4)
benchmarks/bench_qft.pngis excluded by!**/*.pngbenchmarks/bench_qft_evolved.pngis excluded by!**/*.pngbenchmarks/bench_random.pngis excluded by!**/*.pngbenchmarks/bench_random_evolved.pngis excluded by!**/*.png
📒 Files selected for processing (16)
.github/workflows/main.yml.github/workflows/pre-release.yml.github/workflows/release.yml.github/workflows/test-release.ymlCHANGELOG.mdbenchmarks/bench_evolved.jsonbin/cibw/test_wheel.shbin/test_sdist.shpyproject.tomlsetup.pysrc/pyqasm/accelerate/sv_sim.pyxsrc/pyqasm/simulator/__init__.pysrc/pyqasm/simulator/statevector.pytests/test_perf_regression.pytests/test_sv_sim.pytests/test_sv_sim_core.py
✅ Files skipped from review due to trivial changes (2)
- bin/test_sdist.sh
- CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (5)
- src/pyqasm/simulator/init.py
- tests/test_sv_sim.py
- src/pyqasm/simulator/statevector.py
- src/pyqasm/accelerate/sv_sim.pyx
- tests/test_perf_regression.py
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 10
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/main.yml:
- Line 188: The codecov/codecov-action reference in the workflow is using a
version tag (v6.0.0) which is mutable and poses a supply-chain risk. Replace the
version tag with a full commit SHA in the uses statement for
codecov/codecov-action to pin it to an immutable commit, preventing the tag from
being rewritten to malicious code.
In `@bin/cibw/test_wheel.sh`:
- Line 36: The pytest path argument $project/tests in the pytest command is not
quoted, which can cause word splitting and globbing issues if the project path
contains spaces or special characters. Add double quotes around $project/tests
to ensure the path is treated as a single argument, changing it from python -m
pytest $project/tests -m "not benchmark" to properly quote the path variable.
In `@tests/test_sv_sim_core.py`:
- Around line 161-166: The test function `test_single_qubit_gate_action` is
missing a type annotation for its parameter `gate`. Add a type annotation of
`str` to the `gate` parameter since it receives string values from the
parametrize decorator. This will comply with the coding guideline that requires
all functions, methods, and class attributes to have type annotations.
- Around line 60-64: The _embed function is missing both a docstring and type
annotations as required by the coding guidelines. Add type annotations to the
function parameters (mat, target, num_qubits) and the return type to indicate
they are numpy arrays and integers respectively. Additionally, add a
comprehensive docstring at the beginning of the _embed function that explains
its purpose (embedding an operation on a target qubit into a larger Hilbert
space), describes each parameter, and specifies what the function returns.
- Around line 67-78: The function _embed_controlled is missing both type
annotations and a docstring as required by the coding guidelines. Add type
annotations to all parameters (mat, control, target, num_qubits) and to the
return type of the function. Additionally, add a docstring at the beginning of
the function that explains its purpose, describes each parameter with its
expected type and meaning, and describes what the function returns.
- Around line 101-106: The assert_sv_close function is missing type annotations
and has an incomplete docstring. Add type annotations for all parameters
(actual, expected, atol) and the return type to the function signature. Expand
the docstring to document what each parameter represents, what values they
should contain, what the atol parameter controls, and clarify that the function
returns None and performs assertion checks rather than returning a result.
- Around line 123-136: The function test_crz_fast_path_matches_decomposition is
missing type annotations on its parameters. Add type annotations to all function
parameters: theta should be annotated as float, and both control and target
should be annotated as int. This ensures compliance with the coding guideline
requiring all functions to have type annotations.
- Around line 109-112: The `run_sv` function is missing both a docstring and
type annotations as required by coding guidelines. Add type annotations to all
parameters (qasm and external_gates) and the return type to the function
signature. Add a docstring to the function that explains its purpose (what it
does with the qasm string and external gates), describes each parameter and its
expected type, and describes what the function returns (the final statevector).
- Around line 81-98: The reference_statevector function is missing type
annotations and has an incomplete docstring that does not follow Python
conventions. Add type annotations to the function signature for the ops
parameter (a list of tuples containing operation information), the num_qubits
parameter (an integer), and the return type (numpy ndarray). Replace the current
single-line docstring with a properly formatted docstring that includes separate
Args and Returns sections, clearly documenting what each parameter represents
and what the function returns.
- Around line 349-352: The loads_unrolled function is missing both a docstring
and type annotations as required by coding guidelines. Add type annotations for
the qasm parameter and the return type of the loads_unrolled function, and add a
comprehensive docstring that explains the function's purpose, describes the qasm
parameter, and documents the return value (which should be annotated as
QasmModule or the appropriate return type).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e9fd7557-16b1-4b89-be87-9ed5cfe2ff59
⛔ Files ignored due to path filters (4)
benchmarks/bench_qft.pngis excluded by!**/*.pngbenchmarks/bench_qft_evolved.pngis excluded by!**/*.pngbenchmarks/bench_random.pngis excluded by!**/*.pngbenchmarks/bench_random_evolved.pngis excluded by!**/*.png
📒 Files selected for processing (16)
.github/workflows/main.yml.github/workflows/pre-release.yml.github/workflows/release.yml.github/workflows/test-release.ymlCHANGELOG.mdbenchmarks/bench_evolved.jsonbin/cibw/test_wheel.shbin/test_sdist.shpyproject.tomlsetup.pysrc/pyqasm/accelerate/sv_sim.pyxsrc/pyqasm/simulator/__init__.pysrc/pyqasm/simulator/statevector.pytests/test_perf_regression.pytests/test_sv_sim.pytests/test_sv_sim_core.py
✅ Files skipped from review due to trivial changes (2)
- bin/test_sdist.sh
- CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (5)
- src/pyqasm/simulator/init.py
- tests/test_sv_sim.py
- src/pyqasm/simulator/statevector.py
- src/pyqasm/accelerate/sv_sim.pyx
- tests/test_perf_regression.py
🛑 Comments failed to post (10)
.github/workflows/main.yml (1)
188-188:
⚠️ Potential issue | 🟠 Major🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Verify unpinned GitHub Actions references in workflows. rg -nP '^\s*uses:\s*\S+@(?![0-9a-fA-F]{40}\b)\S+' .github/workflowsRepository: qBraid/pyqasm
Length of output: 3426
🌐 Web query:
codecov-action v6.0.0 commit SHA GitHub💡 Result:
The codecov-action version v6.0.0 is associated with the commit SHA 57e3a136b779b570ffcdbf80b3bdc90e7fab3de2 [1][2]. This release was merged into the main branch on March 26, 2026 [1][2]. You can reference this specific version in your GitHub Actions workflow using the following syntax: - uses: codecov/codecov-action@57e3a13 This version introduced support for Node.js 24, which may cause breaking changes for systems that do not yet support it [3][4]. It is recommended to pin your actions to specific commit SHAs for better security and stability [2].
Citations:
- 1: codecov/codecov-action@57e3a13
- 2: codecov/codecov-action#1928
- 3: https://github.com/codecov/codecov-action/releases/tag/v6.0.0
- 4: https://newreleases.io/project/github/codecov/codecov-action/release/v6.0.0
Pin GitHub Action to a full commit SHA.
codecov/codecov-action@v6.0.0is not immutable. Pinning to a commit SHA reduces supply-chain risk.Suggested hardening change
- uses: codecov/codecov-action@v6.0.0 + uses: codecov/codecov-action@57e3a136b779b570ffcdbf80b3bdc90e7fab3de2🧰 Tools
🪛 zizmor (1.25.2)
[error] 188-188: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/main.yml at line 188, The codecov/codecov-action reference in the workflow is using a version tag (v6.0.0) which is mutable and poses a supply-chain risk. Replace the version tag with a full commit SHA in the uses statement for codecov/codecov-action to pin it to an immutable commit, preventing the tag from being rewritten to malicious code.Source: Linters/SAST tools
bin/cibw/test_wheel.sh (1)
36-36:
⚠️ Potential issue | 🟡 Minor🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Verify unquoted path usage in this script. if command -v shellcheck >/dev/null 2>&1; then shellcheck -f gcc bin/cibw/test_wheel.sh else rg -n '\$project/tests' bin/cibw/test_wheel.sh fiRepository: qBraid/pyqasm
Length of output: 241
Quote the pytest path argument.
Use quoting to avoid word splitting/globbing on
$project/tests.Suggested fix
-python -m pytest $project/tests -m "not benchmark" +python -m pytest "$project/tests" -m "not benchmark"📝 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.python -m pytest "$project/tests" -m "not benchmark"🧰 Tools
🪛 Shellcheck (0.11.0)
[info] 36-36: Double quote to prevent globbing and word splitting.
(SC2086)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bin/cibw/test_wheel.sh` at line 36, The pytest path argument $project/tests in the pytest command is not quoted, which can cause word splitting and globbing issues if the project path contains spaces or special characters. Add double quotes around $project/tests to ensure the path is treated as a single argument, changing it from python -m pytest $project/tests -m "not benchmark" to properly quote the path variable.Source: Linters/SAST tools
tests/test_sv_sim_core.py (8)
60-64: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add docstring and type annotations.
This function is missing both a docstring and type annotations, as required by the coding guidelines. As per coding guidelines, "Every module, class, method, and function must have a docstring explaining its purpose, parameters, and return values" and "All functions, methods, and class attributes must have type annotations."
📝 Proposed fix
-def _embed(mat, target, num_qubits): +def _embed(mat: np.ndarray, target: int, num_qubits: int) -> np.ndarray: + """ + Embed a single-qubit gate into a multi-qubit operator. + + Args: + mat: The 2x2 single-qubit gate matrix. + target: The target qubit index. + num_qubits: Total number of qubits. + + Returns: + The full (2^num_qubits x 2^num_qubits) operator matrix. + """ op = np.array([[1]], dtype=complex)📝 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 _embed(mat: np.ndarray, target: int, num_qubits: int) -> np.ndarray: """ Embed a single-qubit gate into a multi-qubit operator. Args: mat: The 2x2 single-qubit gate matrix. target: The target qubit index. num_qubits: Total number of qubits. Returns: The full (2^num_qubits x 2^num_qubits) operator matrix. """ op = np.array([[1]], dtype=complex) for qubit in range(num_qubits): op = np.kron(mat if qubit == target else _I, op) return op🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_sv_sim_core.py` around lines 60 - 64, The _embed function is missing both a docstring and type annotations as required by the coding guidelines. Add type annotations to the function parameters (mat, target, num_qubits) and the return type to indicate they are numpy arrays and integers respectively. Additionally, add a comprehensive docstring at the beginning of the _embed function that explains its purpose (embedding an operation on a target qubit into a larger Hilbert space), describes each parameter, and specifies what the function returns.Source: Coding guidelines
67-78: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add docstring and type annotations.
This function is missing both a docstring and type annotations, as required by the coding guidelines. As per coding guidelines, "Every module, class, method, and function must have a docstring explaining its purpose, parameters, and return values" and "All functions, methods, and class attributes must have type annotations."
📝 Proposed fix
-def _embed_controlled(mat, control, target, num_qubits): +def _embed_controlled(mat: np.ndarray, control: int, target: int, num_qubits: int) -> np.ndarray: + """ + Embed a controlled single-qubit gate into a multi-qubit operator. + + Args: + mat: The 2x2 single-qubit gate matrix to apply when control is |1>. + control: The control qubit index. + target: The target qubit index. + num_qubits: Total number of qubits. + + Returns: + The full (2^num_qubits x 2^num_qubits) controlled operator matrix. + """ p0 = np.array([[1, 0], [0, 0]], dtype=complex)📝 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 _embed_controlled(mat: np.ndarray, control: int, target: int, num_qubits: int) -> np.ndarray: """ Embed a controlled single-qubit gate into a multi-qubit operator. Args: mat: The 2x2 single-qubit gate matrix to apply when control is |1>. control: The control qubit index. target: The target qubit index. num_qubits: Total number of qubits. Returns: The full (2^num_qubits x 2^num_qubits) controlled operator matrix. """ p0 = np.array([[1, 0], [0, 0]], dtype=complex) p1 = np.array([[0, 0], [0, 1]], dtype=complex) op0 = op1 = np.array([[1]], dtype=complex) for qubit in range(num_qubits): if qubit == control: op0, op1 = np.kron(p0, op0), np.kron(p1, op1) elif qubit == target: op0, op1 = np.kron(_I, op0), np.kron(mat, op1) else: op0, op1 = np.kron(_I, op0), np.kron(_I, op1) return op0 + op1🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_sv_sim_core.py` around lines 67 - 78, The function _embed_controlled is missing both type annotations and a docstring as required by the coding guidelines. Add type annotations to all parameters (mat, control, target, num_qubits) and to the return type of the function. Additionally, add a docstring at the beginning of the function that explains its purpose, describes each parameter with its expected type and meaning, and describes what the function returns.Source: Coding guidelines
81-98: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Improve docstring and add type annotations.
The docstring does not follow Python docstring conventions with proper Args/Returns sections, and type annotations are missing. As per coding guidelines, "Every module, class, method, and function must have a docstring explaining its purpose, parameters, and return values" and "All functions, methods, and class attributes must have type annotations."
📝 Proposed fix
-def reference_statevector(ops, num_qubits): - """ops: list of (name, [qubits], [params]). Returns the final statevector.""" +def reference_statevector(ops: list[tuple[str, list[int], list[float]]], num_qubits: int) -> np.ndarray: + """ + Compute the final statevector for a sequence of gate operations. + + Args: + ops: List of (gate_name, qubit_indices, parameters) tuples. + num_qubits: Total number of qubits in the circuit. + + Returns: + The final statevector as a complex numpy array of shape (2^num_qubits,). + + Raises: + ValueError: If an unsupported gate name is encountered. + """ sv = np.zeros(2**num_qubits, dtype=complex)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_sv_sim_core.py` around lines 81 - 98, The reference_statevector function is missing type annotations and has an incomplete docstring that does not follow Python conventions. Add type annotations to the function signature for the ops parameter (a list of tuples containing operation information), the num_qubits parameter (an integer), and the return type (numpy ndarray). Replace the current single-line docstring with a properly formatted docstring that includes separate Args and Returns sections, clearly documenting what each parameter represents and what the function returns.Source: Coding guidelines
101-106: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add type annotations and improve docstring.
Type annotations are missing, and the docstring could be more detailed. As per coding guidelines, "All functions, methods, and class attributes must have type annotations" and docstrings should explain parameters and return values.
📝 Proposed fix
-def assert_sv_close(actual, expected, atol=1e-9): - """Compare statevectors up to an irrelevant global phase.""" +def assert_sv_close(actual: np.ndarray, expected: np.ndarray, atol: float = 1e-9) -> None: + """ + Assert that two statevectors are equal up to a global phase. + + Args: + actual: The computed statevector. + expected: The reference statevector. + atol: Absolute tolerance for the comparison. + + Raises: + AssertionError: If the statevectors differ by more than a global phase or atol. + """ idx = int(np.argmax(np.abs(expected)))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_sv_sim_core.py` around lines 101 - 106, The assert_sv_close function is missing type annotations and has an incomplete docstring. Add type annotations for all parameters (actual, expected, atol) and the return type to the function signature. Expand the docstring to document what each parameter represents, what values they should contain, what the atol parameter controls, and clarify that the function returns None and performs assertion checks rather than returning a result.Source: Coding guidelines
109-112: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add docstring and type annotations.
This helper function is missing both a docstring and type annotations, as required by the coding guidelines. As per coding guidelines, "Every module, class, method, and function must have a docstring explaining its purpose, parameters, and return values" and "All functions, methods, and class attributes must have type annotations."
📝 Proposed fix
-def run_sv(qasm, external_gates=None): +def run_sv(qasm: str, external_gates: list[str] | None = None) -> np.ndarray: + """ + Run the simulator on a QASM string and return the final statevector. + + Args: + qasm: The OpenQASM 3 program string. + external_gates: Optional list of gate names to treat as external (not unrolled). + + Returns: + The final statevector as a complex numpy array. + """ module = loads(qasm)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_sv_sim_core.py` around lines 109 - 112, The `run_sv` function is missing both a docstring and type annotations as required by coding guidelines. Add type annotations to all parameters (qasm and external_gates) and the return type to the function signature. Add a docstring to the function that explains its purpose (what it does with the qasm string and external gates), describes each parameter and its expected type, and describes what the function returns (the final statevector).Source: Coding guidelines
123-136: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add type annotations to test parameters.
The parametrized test function parameters lack type annotations. As per coding guidelines, "All functions, methods, and class attributes must have type annotations."
📝 Proposed fix
`@pytest.mark.parametrize`("theta", [0.0, 0.7, np.pi / 2, np.pi, -1.3, 2 * np.pi]) `@pytest.mark.parametrize`("control, target", [(0, 1), (1, 0)]) -def test_crz_fast_path_matches_decomposition(theta, control, target): +def test_crz_fast_path_matches_decomposition(theta: float, control: int, target: int) -> None: qasm = (📝 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.`@pytest.mark.parametrize`("theta", [0.0, 0.7, np.pi / 2, np.pi, -1.3, 2 * np.pi]) `@pytest.mark.parametrize`("control, target", [(0, 1), (1, 0)]) def test_crz_fast_path_matches_decomposition(theta: float, control: int, target: int) -> None: qasm = ( "OPENQASM 3;\n" 'include "stdgates.inc";\n' "qubit[2] q;\n" "h q[0];\n" "h q[1];\n" f"crz({theta}) q[{control}], q[{target}];\n" ) fast = run_sv(qasm, external_gates=["crz"]) # hits the crz fast path decomposed = run_sv(qasm) # full rz/rx/cx decomposition (oracle) assert_sv_close(fast, decomposed)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_sv_sim_core.py` around lines 123 - 136, The function test_crz_fast_path_matches_decomposition is missing type annotations on its parameters. Add type annotations to all function parameters: theta should be annotated as float, and both control and target should be annotated as int. This ensures compliance with the coding guideline requiring all functions to have type annotations.Source: Coding guidelines
161-166: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add type annotation to test parameter.
The parametrized test function parameter lacks a type annotation. As per coding guidelines, "All functions, methods, and class attributes must have type annotations."
📝 Proposed fix
`@pytest.mark.parametrize`("gate", ["x", "y", "z", "h", "s", "sdg", "t", "tdg", "id"]) -def test_single_qubit_gate_action(gate): +def test_single_qubit_gate_action(gate: str) -> None: qasm = f'OPENQASM 3;\ninclude "stdgates.inc";\nqubit[1] q;\nx q[0];\n{gate} q[0];\n'🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_sv_sim_core.py` around lines 161 - 166, The test function `test_single_qubit_gate_action` is missing a type annotation for its parameter `gate`. Add a type annotation of `str` to the `gate` parameter since it receives string values from the parametrize decorator. This will comply with the coding guideline that requires all functions, methods, and class attributes to have type annotations.Source: Coding guidelines
349-352: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add docstring and type annotations.
This helper function is missing both a docstring and type annotations, as required by the coding guidelines. As per coding guidelines, "Every module, class, method, and function must have a docstring explaining its purpose, parameters, and return values" and "All functions, methods, and class attributes must have type annotations."
📝 Proposed fix
-def loads_unrolled(qasm): +def loads_unrolled(qasm: str): + """ + Load and unroll a QASM program string. + + Args: + qasm: The OpenQASM 3 program string. + + Returns: + The unrolled QasmModule. + """ module = loads(qasm)Note: You may also want to add a return type annotation (likely
QasmModulefrom pyqasm).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_sv_sim_core.py` around lines 349 - 352, The loads_unrolled function is missing both a docstring and type annotations as required by coding guidelines. Add type annotations for the qasm parameter and the return type of the loads_unrolled function, and add a comprehensive docstring that explains the function's purpose, describes the qasm parameter, and documents the return value (which should be annotated as QasmModule or the appropriate return type).Source: Coding guidelines
…rocess-no-cache # Conflicts: # CHANGELOG.md
…atements Fixes several silent wrong-answer bugs found in adversarial review: - Qubit operands were resolved by register-local index alone, so any multi-register program addressed the wrong qubits (e.g. a cross-register 'h a[0]; cx a[0], b[0]' produced no entanglement at all). Operands now resolve through a (register name, local index) -> global index map built from the unrolled AST's declarations, with range validation. - Gates on 3+ qubits fell through the 1/2-qubit dispatch with no else and were silently deleted (every ccx/toffoli, i.e. every Grover/oracle circuit). ccx now emits the standard exact 15-gate decomposition on the fast kernels; any other arity raises. - reset, mid-circuit measurement, and classical control flow were silently discarded; they now raise NotImplementedError. Terminal measurement is accepted (the sampling step honors it). Barriers/declarations remain no-ops. - p/phaseshift/u1, cp/cphaseshift/cu1, sx and sxdg are now supported directly; main's c3x/c4x decompositions emit literal p gates that previously failed with a misleading 'call unroll() first' error. - An un-unrolled QasmModule silently fell back to the raw AST (dropping loops/conditionals); it now raises with a clear message. - measurement_counts keys were the mirror image of the probabilities indexing; both are now little-endian and qiskit-compatible, documented on SimulatorResult. - 2**num_qubits allocation is now guarded (default ceiling 30 qubits, PYQASM_SIM_MAX_QUBITS to override) instead of dying with SIGBUS/OOM mid-kernel; shots are validated before simulation; 0-qubit programs return a trivial result. - numba helpers no longer use fastmath (contradicted the strict-IEEE policy stated in setup.py); the dead two-qubit scratch buffer no longer over-allocates 256 bytes per instruction. All behaviors covered by new qiskit-free regression tests, plus qiskit-backed comparisons for ccx, multi-register layout, and the counts convention; verified against Aer on 300 randomized circuits.
…ing generated C sv_sim.pyx: - Every cpdef entry point now validates its arguments (statevector length vs num_qubits, qubit bounds, control != target, buffer sizes, opcode range) before any raw pointer is taken; malformed calls from Python raise ValueError instead of segfaulting the interpreter. apply_circuit validates all instructions up front. - Dropped the deprecated IF/DEF compile-time conditionals and the openmp cimport. prange is used unconditionally: Cython guards its pragmas with #ifdef _OPENMP, so the same generated C builds serial or parallel purely by compiler flags and no OpenMP flavor is baked into artifacts. - Thread policy no longer reads a per-thread OpenMP ICV (which silently re-enabled parallelism on worker threads and mutated process-global state at import). PYQASM_NUM_THREADS is parsed defensively once into a module-level int and passed explicitly via prange(num_threads=...). setup.py / MANIFEST.in: - cythonize now runs lazily in build_ext.finalize_options, and MANIFEST.in global-excludes *.c, so sdists and wheels ship .pyx sources only (wheel: 805K -> 491K); metadata-only PEP 517 hooks skip codegen. - Windows OpenMP is now opt-in (PYQASM_WINDOWS_OPENMP): /openmp made the extension depend on vcomp140.dll, which delvewheel vendored from whatever PATH offered first on the runner (ImageMagick's copy). - The OpenMP probe honors $CC instead of hardcoding cc. Verified: sdist and wheel built from a clean tree and from the sdist both contain no .c; manylinux_2_28 build with OpenMP repairs to the same manylinux2014/2_17/2_28 tags as before with libgomp vendored; parallel kernels produce identical results on main and worker threads.
…ble benchmarks, docs page - main.yml: write coverage reports to build/coverage/ where the codecov step actually looks for them. - tox.ini: exclude benchmark-marked tests from the default unit-tests run, matching CI. - build_wheels.sh: pin cibuildwheel to >=3,<4 in the release critical path. - benchmarks/: profile_simulator.py no longer imports the removed _fuse_gates (was dead on arrival); qiskit/cirq/pennylane imports are now optional with graceful skips; added --quick/--repeats/--out-dir flags; added README.md documenting how to run each script and the provenance of the committed bench_evolved.json / *_evolved.png artifacts. - docs: add pyqasm.simulator API page and toctree entry (sphinx -W clean).
- bench_simulator.py: align each candidate statevector to PyQASM's global
phase before the correctness allclose; equivalent states differing only
by e^{i*phi} no longer report FAIL.
- test_sv_sim.py: normalize global phase unconditionally instead of
keying on 'global phase' appearing in the test description, with an
explicit unit-modulus assertion so non-phase deviations still fail.
- Remove an implicit string concatenation (W1404) in test_multi_register_distinct_qubits. - Disable no-name-in-module on the two compiled-extension imports of pyqasm.accelerate.sv_sim: against an installed wheel pylint cannot see into the C extension and reported E0611 (locally the editable install resolved it). Verified: pylint exits 0 against a wheel-installed env.
Member
|
@coderabbitai review All three open review threads have been addressed and resolved (phase-invariant comparisons fixed in 3c3a2ba; the PT006 suggestion dismissed as not applicable — this repo lints with pylint/black/isort, no ruff config). Requesting a fresh review to clear the stale changes-requested state. |
|
✏️ Learnings added
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
tests/test_sv_sim_core.py (1)
60-113: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd type annotations and docstrings to the reference-simulator helpers.
_embed,_embed_controlled,run_sv, andloads_unrolled(and the laterbinop,_ry,_reference_ccx) have no type annotations, and several lack docstrings. These are reusable helpers where the signatures matter for mypy and readability.As per coding guidelines: "All functions, methods, and class attributes must have type annotations, adhering to Python typing conventions and ensuring compatibility with mypy" and "Every module, class, method, and function must have a docstring".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_sv_sim_core.py` around lines 60 - 113, Add complete Python type annotations and concise docstrings to the reference-simulator helpers `_embed`, `_embed_controlled`, `run_sv`, and `loads_unrolled`, plus the later `binop`, `_ry`, and `_reference_ccx` helpers. Annotate parameters and return types using typing conventions compatible with mypy, while documenting each helper’s purpose and key arguments without changing behavior.Source: Coding guidelines
setup.py (2)
108-155: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd docstrings and type annotations to
BuildExt.finalize_options/build_extensions.Both overridden methods are undocumented and untyped.
♻️ Proposed fix
- def finalize_options(self): + def finalize_options(self) -> None: + """Cythonize extension sources lazily, then finalize base options.""" # pylint: disable-next=import-outside-toplevel from Cython.Build import cythonize @@ - def build_extensions(self): + def build_extensions(self) -> None: + """Inject portable optimization/OpenMP flags per compiler before building.""" is_msvc = self.compiler.compiler_type == "msvc"As per coding guidelines, "Every module, class, method, and function must have a docstring..." and "All functions, methods, and class attributes must have type annotations...".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@setup.py` around lines 108 - 155, Add concise docstrings and complete type annotations to BuildExt.finalize_options and BuildExt.build_extensions, including explicit return annotations appropriate for setuptools command overrides. Preserve their existing Cythonization and compiler-flag behavior unchanged.Source: Coding guidelines
60-105: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd type annotations; expand the docstring's return description.
_detect_openmp_unixhas no parameter/return type annotations, and its one-line docstring doesn't describe the(compile_args, link_args)return contract.♻️ Proposed fix
-def _detect_openmp_unix(): - """Detect OpenMP availability for GCC/Clang and return (compile_args, link_args).""" +def _detect_openmp_unix() -> tuple[list[str], list[str]]: + """Detect OpenMP availability for GCC/Clang. + + Returns: + A ``(compile_args, link_args)`` tuple; both empty when OpenMP is + unavailable or disabled for the current platform. + """As per coding guidelines, "All functions, methods, and class attributes must have type annotations, adhering to Python typing conventions and ensuring compatibility with mypy" and "Every module, class, method, and function must have a docstring explaining its purpose, parameters, and return values."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@setup.py` around lines 60 - 105, Update _detect_openmp_unix with a return type annotation describing the tuple of string argument lists, and expand its docstring to document that it takes no parameters and returns (compile_args, link_args), or empty lists when OpenMP is unavailable.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@benchmarks/bench_simulator.py`:
- Around line 437-448: Validate the --repeats argument during argument parsing
so only positive integers are accepted, rejecting zero and negative values
before assigning to N_REPEATS. Update the parser configuration for the repeats
argument while preserving the existing default and --quick behavior.
- Around line 47-53: Complete the annotations and docstrings throughout
benchmarks/bench_simulator.py for _median_time, gates_to_qasm, all prepare_* and
bench_* wrappers, available_simulators, _reverse_endian, run_benchmarks, and
main. Add explicit return types matching each function’s behavior and concise
docstrings describing parameters, return values, and purpose, while preserving
existing functionality.
In `@bin/cibw/test_wheel.sh`:
- Around line 31-36: Quote the project path in the pytest invocation by changing
the unquoted $project/tests argument to "$project/tests", preserving the
existing test marker selection.
---
Nitpick comments:
In `@setup.py`:
- Around line 108-155: Add concise docstrings and complete type annotations to
BuildExt.finalize_options and BuildExt.build_extensions, including explicit
return annotations appropriate for setuptools command overrides. Preserve their
existing Cythonization and compiler-flag behavior unchanged.
- Around line 60-105: Update _detect_openmp_unix with a return type annotation
describing the tuple of string argument lists, and expand its docstring to
document that it takes no parameters and returns (compile_args, link_args), or
empty lists when OpenMP is unavailable.
In `@tests/test_sv_sim_core.py`:
- Around line 60-113: Add complete Python type annotations and concise
docstrings to the reference-simulator helpers `_embed`, `_embed_controlled`,
`run_sv`, and `loads_unrolled`, plus the later `binop`, `_ry`, and
`_reference_ccx` helpers. Annotate parameters and return types using typing
conventions compatible with mypy, while documenting each helper’s purpose and
key arguments without changing behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1abdf37a-4535-4296-a0af-556fbfe35c30
⛔ Files ignored due to path filters (4)
benchmarks/bench_qft.pngis excluded by!**/*.pngbenchmarks/bench_qft_evolved.pngis excluded by!**/*.pngbenchmarks/bench_random.pngis excluded by!**/*.pngbenchmarks/bench_random_evolved.pngis excluded by!**/*.png
📒 Files selected for processing (25)
.github/workflows/main.yml.github/workflows/pre-release.yml.github/workflows/release.yml.github/workflows/test-release.ymlCHANGELOG.mdMANIFEST.inbenchmarks/README.mdbenchmarks/bench_evolved.jsonbenchmarks/bench_simulator.pybenchmarks/plot_benchmarks.pybenchmarks/profile_simulator.pybin/cibw/build_wheels.shbin/cibw/test_wheel.shbin/test_sdist.shdocs/api/pyqasm.simulator.rstdocs/index.rstpyproject.tomlsetup.pysrc/pyqasm/accelerate/sv_sim.pyxsrc/pyqasm/simulator/__init__.pysrc/pyqasm/simulator/statevector.pytests/test_perf_regression.pytests/test_sv_sim.pytests/test_sv_sim_core.pytox.ini
🚧 Files skipped from review as they are similar to previous changes (6)
- src/pyqasm/simulator/init.py
- pyproject.toml
- tests/test_perf_regression.py
- benchmarks/plot_benchmarks.py
- benchmarks/profile_simulator.py
- src/pyqasm/simulator/statevector.py
…icks - bench_simulator.py: --repeats now rejects zero/negative values at parse time via an argparse type validator (default and --quick behavior unchanged). - test_wheel.sh: quote "$project/tests" so paths with spaces survive. - setup.py: return annotations and docstrings on _detect_openmp_unix and the BuildExt overrides (behavior unchanged; wheel rebuild verified).
…eRabbit) Adds a GateSpec type alias, explicit parameter/return annotations, and concise docstrings to _median_time, gates_to_qasm, the prepare_*/bench_* wrappers, available_simulators, _reverse_endian, run_benchmarks, and main, per the CONTRIBUTING docstring checklist. No behavior change; --quick runs of bench_simulator.py and plot_benchmarks.py verified.
Member
|
@coderabbitai approve All review feedback has been addressed across dfb2b03/a39d9a4 and every review thread is resolved; CI is fully green (26/26). The outstanding changes-requested review predates these fixes. |
|
✅ Action performedComments resolved. Approval is disabled; enable |
ryanhill1
dismissed
coderabbitai[bot]’s stale review
July 22, 2026 17:02
Stale (2026-05-26, pre-dates the fix commits). All CodeRabbit findings addressed and threads resolved; CI 26/26 green. CodeRabbit itself confirmed: 'Approved — the resolved feedback and green CI support clearing the stale changes-requested state' but its config (reviews.request_changes_workflow disabled) prevents it from submitting the approval.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR targets
mainfrom the currentsvbranch work plus one additional commit that ports the honest, non-cache AlphaEvolve Run 06 statevector improvements.It deliberately does not include the evolved
_PROGRAM_INSTRUCTION_CACHE/ repeated-QASM memoization path, since that overfit the AlphaEvolve repeated-timing harness. The simulator still preprocesses each input on everySimulator.run(...)call.Included from
svsv_simkernelsAdditional evolved changes
simulation = ["numba>=0.59"]optional extra; fallback shim keeps numba optionalBenchmark artifacts updated
The existing default benchmark plots were regenerated from this PR branch with the improved simulator:
benchmarks/bench_random.pngbenchmarks/bench_qft.pngThe benchmarks folder also includes explicit no-cache before/after comparison artifacts:
benchmarks/bench_evolved_no_cache.jsonbenchmarks/bench_random_evolved_no_cache.pngbenchmarks/bench_qft_evolved_no_cache.pngThe comparison artifacts use pre-unrolled
QasmModuleinputs and intentionally do not include the repeated-QASM cache path.Validation
Performed locally against the installed
svbranch kernels from the AlphaEvolve workspace:QasmModulepathshots > 0smoke testatol=1e-10Regenerated default benchmark plots with:
No-cache comparison artifacts (
PYQASM_NUM_THREADS=1, median of 5):atol=1e-10AlphaEvolve full overlay results from the experiment showed the graph-compatible non-cache path at ~1.23x random and ~1.36x QFT geomean; this PR's committed benchmark artifacts were regenerated from the PR branch implementation.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests