parallelized MPO equivalence checker#448
Merged
Merged
Conversation
Member
Author
|
@CodeRabbit review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
📝 WalkthroughSummary by CodeRabbitRelease Notes
WalkthroughThis PR introduces dual-backend quantum circuit equivalence checking (matrix and MPO) with automatic backend selection, parallel MPO execution via thread pools, shared parallel utilities, refactored MPO identity construction, and comprehensive tests and documentation covering backend behavior, parallelism, and auto-selection. ChangesDual-backend equivalence checking with parallel MPO
Sequence Diagram(s)(Diagrams embedded in the hidden review stack artifact above show backend selection flow, parallel MPO execution, and matrix operator computation path.) Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes The PR introduces substantial new functionality across multiple modules: a new dense-matrix backend with tensor contractions, parallel MPO execution with thread-pool orchestration, refactored parallel utilities, and dual-backend orchestration in Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches✨ Simplify code
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 12
🤖 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 `@docs/examples/equivalence_checking.md`:
- Around line 203-205: Update the wording to match the implemented cutoff: state
that MPO parallelization (when parallel=True) is enabled for systems with 13+
qubits and that 12 qubits and below remain serial; replace the phrases "12+
qubits" and "Below 12 qubits" with "13+ qubits" and "12 qubits and below" and
mention `parallel=True`/MPO parallelization accordingly so the docs match the PR
behavior.
- Around line 82-86: The Sphinx {func} cross-reference points to the moved
helper; update or remove it: replace
`{func}`~mqt.yaqs.core.parallel.available_cpus with the correct symbol in its
new module (e.g., the function in parallel_utils, such as
`mqt.yaqs.core.parallel_utils.available_cpus`) if that helper is part of the
public API, or remove the cross-reference and use plain text (e.g., "defaults to
the machine CPU count") if the helper is internal/not documented; ensure the
final docs build and linkcheck pass.
In `@src/mqt/yaqs/digital/utils/matrix_utils.py`:
- Around line 40-58: Update the helper docstrings to Google style by adding an
Args: section for each function (e.g., _circuit_without_measurements,
_identity_operator_tensor, _embed_unitary and the other helpers referenced at
lines 67-79, 97-110, 144-155, 169-174, 194-198, 211-215, 222-226), and keep the
existing one-line summary and Returns: blocks; for example document parameter
names, types and short descriptions (e.g., circuit: QuantumCircuit – the circuit
to copy; num_qubits: int – number of qubits; local: NDArray[np.complex128] –
k-qubit unitary; sites: list[int] – target qubit indices), making each docstring
follow the repository’s Google-style format.
- Around line 267-287: The check_operator_is_identity function incorrectly
rounds np.abs(trace) before normalizing, causing false positives; remove the
pre-comparison rounding and compare the raw normalized absolute trace against
fidelity. Update check_operator_is_identity to compute normalized =
np.abs(trace) / hilbert_dim and return normalized >= fidelity (or equivalently
keep the existing boolean logic but without np.round) so Rz-like near-identity
gates are evaluated correctly.
- Around line 235-255: The function build_composed_operator_tensor currently
assumes both circuits have the same number of qubits but does not enforce it;
add an explicit check comparing circuit1.num_qubits and circuit2.num_qubits at
the start of the function (after reading num_qubits) and raise a ValueError with
a clear message (similar to EquivalenceChecker.check) if they differ so callers
get a deterministic error instead of implicit shape/index failures; locate this
in build_composed_operator_tensor near where num_qubits is obtained and before
any tensor/dag construction.
- Around line 169-184: The current _pop_front_layer_gates silently removes
DAGOpNode measurement nodes (node.op.name == "measure"), which hides mid-circuit
measurements and turns non-unitary circuits into unitary ones; instead, detect
and reject non-final measurements: when you see a "measure" DAGOpNode in
_pop_front_layer_gates, determine whether it is a final measurement (no
subsequent operations use the measured classical/quantum wire) and if not, raise
a clear exception (e.g., ValueError) describing that mid-circuit measurements
are unsupported; only allow removal of truly final measurement nodes (otherwise
keep existing dag.remove_op_node behavior for final measurements) so callers
like equivalence_checker no longer get false positives.
In `@src/mqt/yaqs/digital/utils/mpo_utils.py`:
- Around line 607-609: The ThreadPoolExecutor is currently passed
parallel_worker_init (designed for process pools) which calls process-wide setup
like limit_worker_threads; remove the initializer when creating the thread pool
to avoid repeated process-global changes: modify the ThreadPoolExecutor(...)
call that currently uses initializer=parallel_worker_init (the block that sets
workers = max_workers if max_workers is not None else available_cpus() and then
calls with ThreadPoolExecutor(... ) as pool) to omit the initializer argument
and leave thread-local setup out, or if thread-local initialization is actually
required, replace parallel_worker_init with a new thread-specific initializer
function and use that instead; ensure _consume_dags(pool) still receives the
pool unchanged.
In `@src/mqt/yaqs/equivalence_checker.py`:
- Around line 39-54: Add Google-style docstrings to the new validator and
resolver functions: _validate_representation, _validate_matrix_max_qubits, and
_resolve_representation; each docstring should briefly describe the function
purpose, list parameters (name and type), describe the return type, and document
raised exceptions (e.g., ValueError, TypeError) and conditions under which
they're raised so the API/validation behavior is discoverable and consistent
with the module.
In `@src/mqt/yaqs/parallel_utils.py`:
- Around line 142-150: Add a Google-style "Args" section to the _run_on_executor
docstring describing the parameters: executor (ProcessPoolExecutor used to
submit tasks), tasks (list[TArg] of inputs to process), and worker_fn
(Callable[[TArg], TRes] - the function executed for each task), keeping the
existing Returns section intact; place the Args section directly under the short
description and before Returns in the _run_on_executor docstring so it follows
Google-style formatting.
- Around line 152-160: The results list is initialized with None placeholders
and then filtered before return, which hides missing results and mismatches the
declared return type list[TRes]; update the logic in the function that uses
results/future_to_index/worker_fn to either (A) assert that all entries in
results are non-None after the wait loop (raise an exception or log and fail if
any None remains) and then return results cast to list[TRes], or (B) change the
function signature and return type to list[Optional[TRes]] to reflect possible
partial results; pick one approach and apply it consistently (adjust type hints
and any callers if you choose B, or add an explicit check and clear error
handling if you choose A).
In `@tests/digital/utils/test_equivalence_parallel.py`:
- Around line 78-117: The tests currently set parallel=True on small circuits so
the MPO parallel path in EquivalenceChecker is never exercised; update the test
file to include at least one test that triggers the real parallel code by
creating a circuit with >=13 qubits (e.g., build qc1 and qc2 with 13+ qubits and
a nontrivial gate pattern) and call EquivalenceChecker(representation="mpo",
parallel=True, max_workers=2).check(qc1, qc2) asserting equivalence, or
alternatively mock/override the MPO parallel cutoff used by EquivalenceChecker
(identify and patch the cutoff constant or method inside the EquivalenceChecker
class) for these tests so the current small circuits exercise the parallel
thread-pool code path.
In `@tests/test_equivalence_checker.py`:
- Around line 171-183: The test currently only verifies global-phase equivalence
for representation="matrix" (test_global_phase_equivalence_matrix); update it to
run for both backends by parameterizing the test over representations ["matrix",
"mpo"] (e.g., use pytest.mark.parametrize on a parameter like representation)
and construct EquivalenceChecker(representation=representation, fidelity=1 -
1e-13) inside the test; keep the same circuit setup and assertions
(result["equivalent"] is True and result["representation"] == representation) so
the public contract is validated for both implementations.
🪄 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: 4e4c16c6-7a58-40da-a437-af3f363ab359
📒 Files selected for processing (19)
CHANGELOG.mddocs/examples/equivalence_checking.mddocs/examples/simulator_initialization.mdsrc/mqt/yaqs/__init__.pysrc/mqt/yaqs/core/data_structures/mpo.pysrc/mqt/yaqs/digital/utils/dag_utils.pysrc/mqt/yaqs/digital/utils/equivalence_parallel.pysrc/mqt/yaqs/digital/utils/equivalence_schedule.pysrc/mqt/yaqs/digital/utils/matrix_utils.pysrc/mqt/yaqs/digital/utils/mpo_utils.pysrc/mqt/yaqs/equivalence_checker.pysrc/mqt/yaqs/parallel_utils.pysrc/mqt/yaqs/simulator.pytests/analog/test_lindblad.pytests/analog/test_mcwf.pytests/core/data_structures/test_mpo.pytests/digital/utils/test_equivalence_parallel.pytests/digital/utils/test_mpo_utils.pytests/test_equivalence_checker.py
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Member
Author
|
@CodeRabbit review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
docs/examples/equivalence_checking.md (1)
57-59:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDocument the matrix measurement behavior precisely.
This currently says measurements are ignored on the matrix path, but the new matrix tests only permit final measurements to be stripped; mid-circuit measurements are rejected. Please tighten this wording so users do not expect measured circuits to always be accepted.
🤖 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 `@docs/examples/equivalence_checking.md` around lines 57 - 59, Update the wording around the "matrix path" and `fidelity` to state precisely that only final measurements are stripped before building the unitary U and that mid-circuit (non-final) measurements are rejected by the matrix tests; keep the note that barriers are ignored for the matrix path and retain the MPO algorithm reference (`MPO algorithm`, `sander2025_EquivalenceChecking`) but clarify that the DAG-based MPO processing only applies when no mid-circuit measurements are present.src/mqt/yaqs/parallel_utils.py (2)
84-90:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDocument
mp_contextin Google style.
get_parallel_context()is a new public helper, but its docstring only has a summary. AddArgs:andReturns:so the API docs stay consistent with the rest of the Python surface. As per coding guidelines,**/*.py: Use Google-style docstrings in Python code.🤖 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 `@src/mqt/yaqs/parallel_utils.py` around lines 84 - 90, Add a Google-style docstring to get_parallel_context(mp_context: MPContext = "auto") describing the argument and return value: under "Args:" document mp_context (accepted values like "auto" or a multiprocessing context name and behavior when "auto" chooses "fork" on linux or "spawn" elsewhere), and under "Returns:" state that the function returns a multiprocessing.context.BaseContext used for worker processes; keep the existing one-line summary and ensure types and behavior match the function implementation and MPContext typing.
103-106:⚠️ Potential issue | 🟠 Major | ⚡ Quick winOverwrite the thread-limit env vars instead of using
setdefault.If the parent process already exported
OMP_NUM_THREADS,MKL_NUM_THREADS, or similar, this helper leaves those higher values untouched, so the MPO parallel path can still oversubscribe cores under SLURM/CI shells. That defeats the point of callinglimit_worker_threads(1)before creating workers.Suggested fix
- for key in THREAD_ENV_VARS: - os.environ.setdefault(key, str(n_threads)) - os.environ.setdefault("OMP_DYNAMIC", "FALSE") - os.environ.setdefault("MKL_DYNAMIC", "FALSE") + for key in THREAD_ENV_VARS: + os.environ[key] = str(n_threads) + os.environ["OMP_DYNAMIC"] = "FALSE" + os.environ["MKL_DYNAMIC"] = "FALSE"🤖 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 `@src/mqt/yaqs/parallel_utils.py` around lines 103 - 106, The loop in parallel_utils.py uses os.environ.setdefault for THREAD_ENV_VARS and for "OMP_DYNAMIC"/"MKL_DYNAMIC", which leaves existing parent-exported thread limits in place; change these to overwrite the environment variables (use os.environ[key] = str(n_threads] for each key in THREAD_ENV_VARS and os.environ["OMP_DYNAMIC"] = "FALSE", os.environ["MKL_DYNAMIC"] = "FALSE") so calling limit_worker_threads(1) actually enforces the limits before spawning workers (refer to THREAD_ENV_VARS and the surrounding code that calls limit_worker_threads(1) to locate and update the assignments).src/mqt/yaqs/digital/utils/mpo_utils.py (2)
150-165:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winExpand the new helper docstring to Google style.
apply_temporal_zone_gates()takes multiple parameters but only documents the return value. Please addArgs:fortheta,gates,qubits, andconjugateso this public helper follows the repo standard. As per coding guidelines,**/*.py: Use Google-style docstrings in Python code.🤖 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 `@src/mqt/yaqs/digital/utils/mpo_utils.py` around lines 150 - 165, The docstring for apply_temporal_zone_gates is incomplete: update the Google-style docstring for the function apply_temporal_zone_gates to include an Args: section documenting theta (NDArray[np.complex128], the local tensor to be updated), gates (list[BaseGate], the pre-extracted gates to apply), qubits (list[int], the local qubit indices, using qubits[0] as n), and conjugate (bool, optional, whether to apply the conjugate), and keep/adjust the existing Returns: block to describe the returned NDArray; follow the repository's Google-style wording and type conventions used elsewhere in mpo_utils.py.
273-278:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winAlways conjugate
dag2zones in nearest-neighbor updates.Both call sites currently derive
apply_conjugate_on_secondfrombool(gates1). That means a pair with onlydag2work is applied asU2, notU2†, even thoughapply_long_range_layer()always conjugates the second circuit. The nearest-neighbor MPO path can therefore return a different equivalence result purely because the same gates were scheduled into different zones.Suggested fix
work_items.append( _PairUpdateWork( site=n, gates1=tuple(gates1), gates2=tuple(gates2), - apply_conjugate_on_second=bool(gates1), + apply_conjugate_on_second=bool(gates2), ) )- apply_conjugate = bool(gates1) + apply_conjugate = bool(gates2) mpo.tensors[n], mpo.tensors[n + 1] = compute_pair_update( mpo.tensors[n], mpo.tensors[n + 1], gates1, gates2,Also applies to: 336-344
🤖 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 `@src/mqt/yaqs/digital/utils/mpo_utils.py` around lines 273 - 278, The code sets apply_conjugate_on_second based on bool(gates1) which causes zones containing only dag2 to be applied without conjugation; change the logic to always conjugate the second zone by using apply_conjugate_on_second=bool(gates2) (replace occurrences in the _PairUpdateWork calls around the shown block and the other instance at the 336-344 region) so the second circuit segment is conjugated consistently with apply_long_range_layer().src/mqt/yaqs/equivalence_checker.py (1)
97-126:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate
max_workerseagerly inEquivalenceChecker.__init__
ThreadPoolExecutorrequiresmax_workers > 0; passing0, a negative integer, orFalse(treated as0) raisesValueErrorwhen the MPO backend creates the executor. Reject these values during construction so the public API fails fast with a clear error.Suggested fix
+def _validate_max_workers(max_workers: int | None) -> int | None: + """Validate the MPO thread-pool worker cap. + + Args: + max_workers: Requested worker count, or ``None`` for automatic sizing. + + Returns: + The validated worker count or ``None``. + + Raises: + TypeError: If ``max_workers`` is neither ``None`` nor an ``int``. + ValueError: If ``max_workers`` is less than 1. + """ + if max_workers is None: + return None + if isinstance(max_workers, bool) or not isinstance(max_workers, int): + msg = f"max_workers must be int or None, got {type(max_workers).__name__}." + raise TypeError(msg) + if max_workers < 1: + msg = f"max_workers must be positive, got {max_workers}." + raise ValueError(msg) + return max_workers + ... - self.max_workers = max_workers + self.max_workers = _validate_max_workers(max_workers)🤖 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 `@src/mqt/yaqs/equivalence_checker.py` around lines 97 - 126, EquivalenceChecker.__init__ must validate the max_workers parameter eagerly because ThreadPoolExecutor requires max_workers > 0; add an early check in __init__ that if max_workers is not None and (not an int or <= 0) then raise a ValueError with a clear message; ensure the check also rejects False (which equals 0) so pass-through booleans/invalid types are disallowed, and keep the existing attributes (self.max_workers, self.parallel, etc.) unchanged otherwise.
♻️ Duplicate comments (1)
docs/examples/equivalence_checking.md (1)
200-202:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winThe parallel cutoff wording is still off by one.
The runtime contract in this PR is “above 12 qubits,” but this still documents “12+ qubits” and “Below 12 qubits.” Please change this to
13+ qubitsand12 qubits and belowso the docs match the implementation.🤖 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 `@docs/examples/equivalence_checking.md` around lines 200 - 202, Update the wording to match the runtime cutoff: change "12+ qubits" to "13+ qubits" and change "Below 12 qubits" to "12 qubits and below" in the equivalence_checking.md text that references the parallel cutoff (the sentence mentioning `parallel=True` and thread overhead); ensure both occurrences in that paragraph are updated so the docs reflect the implementation's "above 12 qubits" contract.
🤖 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.
Outside diff comments:
In `@docs/examples/equivalence_checking.md`:
- Around line 57-59: Update the wording around the "matrix path" and `fidelity`
to state precisely that only final measurements are stripped before building the
unitary U and that mid-circuit (non-final) measurements are rejected by the
matrix tests; keep the note that barriers are ignored for the matrix path and
retain the MPO algorithm reference (`MPO algorithm`,
`sander2025_EquivalenceChecking`) but clarify that the DAG-based MPO processing
only applies when no mid-circuit measurements are present.
In `@src/mqt/yaqs/digital/utils/mpo_utils.py`:
- Around line 150-165: The docstring for apply_temporal_zone_gates is
incomplete: update the Google-style docstring for the function
apply_temporal_zone_gates to include an Args: section documenting theta
(NDArray[np.complex128], the local tensor to be updated), gates (list[BaseGate],
the pre-extracted gates to apply), qubits (list[int], the local qubit indices,
using qubits[0] as n), and conjugate (bool, optional, whether to apply the
conjugate), and keep/adjust the existing Returns: block to describe the returned
NDArray; follow the repository's Google-style wording and type conventions used
elsewhere in mpo_utils.py.
- Around line 273-278: The code sets apply_conjugate_on_second based on
bool(gates1) which causes zones containing only dag2 to be applied without
conjugation; change the logic to always conjugate the second zone by using
apply_conjugate_on_second=bool(gates2) (replace occurrences in the
_PairUpdateWork calls around the shown block and the other instance at the
336-344 region) so the second circuit segment is conjugated consistently with
apply_long_range_layer().
In `@src/mqt/yaqs/equivalence_checker.py`:
- Around line 97-126: EquivalenceChecker.__init__ must validate the max_workers
parameter eagerly because ThreadPoolExecutor requires max_workers > 0; add an
early check in __init__ that if max_workers is not None and (not an int or <= 0)
then raise a ValueError with a clear message; ensure the check also rejects
False (which equals 0) so pass-through booleans/invalid types are disallowed,
and keep the existing attributes (self.max_workers, self.parallel, etc.)
unchanged otherwise.
In `@src/mqt/yaqs/parallel_utils.py`:
- Around line 84-90: Add a Google-style docstring to
get_parallel_context(mp_context: MPContext = "auto") describing the argument and
return value: under "Args:" document mp_context (accepted values like "auto" or
a multiprocessing context name and behavior when "auto" chooses "fork" on linux
or "spawn" elsewhere), and under "Returns:" state that the function returns a
multiprocessing.context.BaseContext used for worker processes; keep the existing
one-line summary and ensure types and behavior match the function implementation
and MPContext typing.
- Around line 103-106: The loop in parallel_utils.py uses os.environ.setdefault
for THREAD_ENV_VARS and for "OMP_DYNAMIC"/"MKL_DYNAMIC", which leaves existing
parent-exported thread limits in place; change these to overwrite the
environment variables (use os.environ[key] = str(n_threads] for each key in
THREAD_ENV_VARS and os.environ["OMP_DYNAMIC"] = "FALSE",
os.environ["MKL_DYNAMIC"] = "FALSE") so calling limit_worker_threads(1) actually
enforces the limits before spawning workers (refer to THREAD_ENV_VARS and the
surrounding code that calls limit_worker_threads(1) to locate and update the
assignments).
---
Duplicate comments:
In `@docs/examples/equivalence_checking.md`:
- Around line 200-202: Update the wording to match the runtime cutoff: change
"12+ qubits" to "13+ qubits" and change "Below 12 qubits" to "12 qubits and
below" in the equivalence_checking.md text that references the parallel cutoff
(the sentence mentioning `parallel=True` and thread overhead); ensure both
occurrences in that paragraph are updated so the docs reflect the
implementation's "above 12 qubits" contract.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 94cd86a0-4e02-4f4e-a905-7a9e308cbb45
📒 Files selected for processing (8)
docs/examples/equivalence_checking.mdsrc/mqt/yaqs/digital/utils/matrix_utils.pysrc/mqt/yaqs/digital/utils/mpo_utils.pysrc/mqt/yaqs/equivalence_checker.pysrc/mqt/yaqs/parallel_utils.pytests/digital/utils/test_equivalence_parallel.pytests/digital/utils/test_matrix_utils.pytests/test_equivalence_checker.py
Member
Author
|
@CodeRabbit review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/mqt/yaqs/digital/utils/mpo_utils.py (1)
584-585:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winStale docstring: thread pool, not process pool.
iteratenow uses aThreadPoolExecutor, but the docstring still describesmax_workersas "Worker process count" andmp_contextas the context "for the process pool". Align the wording with the thread-based implementation (and notemp_contextis currently unused/reserved, as inapply_layer).📝 Proposed doc fix
- parallel: If True, parallelize tensor updates within checkerboard sweeps. - max_workers: Worker process count when ``parallel`` is True. - mp_context: Multiprocessing context for the process pool. + parallel: If True, parallelize tensor updates within checkerboard sweeps. + max_workers: Worker thread count when ``parallel`` is True. + mp_context: Reserved (a thread pool is used for MPO parallelism).🤖 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 `@src/mqt/yaqs/digital/utils/mpo_utils.py` around lines 584 - 585, The docstring for iterate is stale: update the descriptions for max_workers and mp_context to reflect that iterate uses a ThreadPoolExecutor (thread pool) not a process pool; change "Worker process count" to "Worker thread count (max_workers for ThreadPoolExecutor)" and change "mp_context: Multiprocessing context for the process pool" to note that mp_context is unused/reserved (like in apply_layer) or removed if not used, and ensure the docstring references the iterate function and the max_workers and mp_context parameters accordingly.
🤖 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.
Outside diff comments:
In `@src/mqt/yaqs/digital/utils/mpo_utils.py`:
- Around line 584-585: The docstring for iterate is stale: update the
descriptions for max_workers and mp_context to reflect that iterate uses a
ThreadPoolExecutor (thread pool) not a process pool; change "Worker process
count" to "Worker thread count (max_workers for ThreadPoolExecutor)" and change
"mp_context: Multiprocessing context for the process pool" to note that
mp_context is unused/reserved (like in apply_layer) or removed if not used, and
ensure the docstring references the iterate function and the max_workers and
mp_context parameters accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3b824516-afb4-4936-b54f-43346efb118c
📒 Files selected for processing (10)
docs/examples/equivalence_checking.mdsrc/mqt/yaqs/digital/utils/matrix_utils.pysrc/mqt/yaqs/digital/utils/mpo_utils.pysrc/mqt/yaqs/digital/utils/scheduler_utils.pysrc/mqt/yaqs/equivalence_checker.pysrc/mqt/yaqs/parallel_utils.pytests/digital/utils/test_matrix_utils.pytests/digital/utils/test_mpo_utils.pytests/digital/utils/test_scheduler_utils.pytests/test_equivalence_checker.py
💤 Files with no reviewable changes (1)
- src/mqt/yaqs/digital/utils/scheduler_utils.py
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
1 participant
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.
Description
This PR parallelizes the MPO equivalence checking method such that disjoint temporal zones are applied at once.
This significantly speeds up equivalence checking for larger systems, although this method scales favorably even in serial.
Parallelization only kicks in above 12 qubits since the thread overhead below this is too high to justify using it based on experiments.
Checklist
If PR contains AI-assisted content:
Assisted-by: [Model Name] via [Tool Name]footer.