✨ Add Hybrid Mapping Extension + Hybrid Synthesis #832
Conversation
…hers are forbidden.
…t if staying still to avoid problems while MoveGroup generation
…ssible positions.
There was a problem hiding this comment.
Pull request overview
This PR extends the Hybrid Mapper for neutral atom quantum computers by adding:
- Extended hybrid mapping with support for bridge gates, multiple initial mapping strategies, enhanced atom movements (passby and flying ancillas), and various bugfixes
- Hybrid synthesis functionality for evaluating and selecting optimal circuits from multiple candidates
- NaViz export support for visualization (.naviz and .namachine formats)
- Comprehensive documentation and Python bindings
Reviewed changes
Copilot reviewed 41 out of 42 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| test/python/hybrid_mapper/test_hybrid_synthesis.py | New tests for hybrid synthesis mapper with multiple architectures |
| test/python/hybrid_mapper/test_hybrid_mapper.py | Updated tests using MapperParameters instead of HybridMapperParameters, added QASM export validation |
| test/hybridmap/test_utils.cpp | New utility tests for coordinate mapping, move vectors, and bridge circuits |
| test/hybridmap/test_scheduler.cpp | New scheduler result serialization tests |
| test/hybridmap/test_mapping.cpp | New tests for mapping exceptions and edge cases |
| test/hybridmap/test_hybridmap.cpp | Extensive refactoring with new parametrized tests, exception tests, and output validation |
| test/hybridmap/test_hybrid_synthesis_map.cpp | New C++ tests for hybrid synthesis mapper functionality |
| test/hybridmap/test_hardware_qubits.cpp | New tests for hardware qubit management and pathfinding |
| test/hybridmap/test_architecture.cpp | New architecture loading, parsing, and animation tests |
| test/hybridmap/circuits/*.qasm | New test circuit files for multi-qubit and long random scenarios |
| test/hybridmap/architectures/*.json | Updated architecture configurations with new parameters |
| src/hybridmap/*.cpp | Major implementation updates including bridge gates, flying ancillas, passby moves, NaViz support, and refactored scheduling/mapping logic |
| include/hybridmap/*.hpp | Updated headers with comprehensive documentation, new data structures (BridgeCircuits, FlyingAncilla, PassByComb), and additional mapping methods |
| python/mqt/qmap/hybrid_mapper.pyi | Updated Python bindings reflecting renamed classes and new synthesis mapper interface |
| src/hybridmap/CMakeLists.txt | Added spdlog dependency |
| docs/index.md | Added reference to new hybrid mapper documentation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
include/hybridmap/Mapping.hpp (1)
170-178: Linear search ingetCircQubitis acceptable but could be optimized.The current implementation performs a linear search through
circToHwto find the circuit qubit for a given hardware qubit. This is O(n) where n is the number of mapped qubits. For typical circuit sizes this is fine, but if performance becomes a concern, consider maintaining a reverse mapping (hwToCirc).src/hybridmap/HybridNeutralAtomMapper.cpp (1)
1386-1454: Minor: dead local and soft shuttling‑layer limit in move‑comb generation.
getMoveCombinationsToPositiondeclares aMoveCombs const moveCombinations;local that is never used; it can be dropped. Also,getAllMoveCombinationschecksallMoves.size() > limitShuttlingLayerbefore adding the newmoves, so the effective limit can be exceeded by the size of the last batch. If you need a hard cap, consider checking again afteraddMoveCombsor changing the condition to>=and/or bounding the number of added combinations.Also applies to: 1456-1476
include/hybridmap/HardwareQubits.hpp (1)
369-375:getInitHwPosreturns a copy, which is safe but has overhead.The method constructs and returns a new
std::mapeach call. This is safe but creates overhead. If called frequently in hot paths, consider caching the result or returning a const reference toinitialHwPosdirectly.
♻️ Duplicate comments (11)
src/hybridmap/Mapping.cpp (1)
51-52: Semantic type mismatch inhwIndicespersists.
hwIndicesis declared asstd::vector<HwQubit>but stores circuit qubit indices (qi,qnof typeqc::Qubit). WhileinvalidCircis correctly typed asqc::Qubit, the container type should match:- std::vector<HwQubit> hwIndices(hwQubits.getNumQubits(), invalidCirc); + std::vector<qc::Qubit> hwIndices(hwQubits.getNumQubits(), invalidCirc);This preserves semantic clarity even if the underlying types are identical.
include/hybridmap/HybridSynthesisMapper.hpp (2)
118-122: saveSynthesizedQc silently ignores write failures.The function doesn't check if the file was successfully opened or if writing succeeded. Consider adding a basic check:
[[maybe_unused]] void saveSynthesizedQc(const std::string& filename) const { std::ofstream ofs(filename); + if (!ofs) { + throw std::runtime_error("Cannot open file for writing: " + filename); + } synthesizedQc.dumpOpenQASM(ofs, false); ofs.close(); }
77-85: Consider documenting single-use intent or resetting base state.
initMappingresets local state (mappedQc,synthesizedQc,mapping) but doesn't reset inherited state (e.g.,stats,mappedQcAOD). If instances are intended for single-run use, document this constraint. Otherwise, consider calling a base-class reset.void initMapping(const size_t nQubits) { if (nQubits > arch->getNpositions()) { throw std::runtime_error("Not enough qubits in architecture."); } + // Reset inherited state for reuse + stats = MapperStats(); + mappedQcAOD = qc::QuantumComputation(arch->getNpositions()); mappedQc = qc::QuantumComputation(arch->getNpositions()); synthesizedQc = qc::QuantumComputation(nQubits); mapping = Mapping(nQubits); initialized = true; }src/hybridmap/NeutralAtomLayer.cpp (1)
31-37:initAllQubitsassumesdag.size()equals number of qubits.This has been flagged in a previous review. The assumption that
dagis qubit-indexed anddag.size()equals the qubit count should be documented or use an explicit qubit count source.include/hybridmap/NeutralAtomLayer.hpp (1)
91-98: Consider reservingendsvector for consistency.The constructor reserves space for
iteratorsandcandidatesbut not forends. While this is a minor optimization concern, addingends.reserve(dag.size())would be consistent with the other reservations.explicit NeutralAtomLayer(DAG graph, const bool isFrontLayer, const uint32_t lookaheadDepth = 1) : dag(std::move(graph)), lookaheadDepth(lookaheadDepth), isFrontLayer(isFrontLayer) { iterators.reserve(dag.size()); candidates.reserve(dag.size()); + ends.reserve(dag.size()); for (auto& i : dag) {include/hybridmap/Mapping.hpp (1)
23-28: Missing standard library headers for self-contained compilation.The header uses
std::deque,std::unique_ptr(line 41), andstd::ranges::any_of(line 188) but doesn't include<deque>,<memory>, or<ranges>. While transitive includes may work on some compilers, explicitly including these headers makes the file self-contained and more portable.#include <algorithm> #include <cstddef> +#include <deque> +#include <memory> +#include <ranges> #include <set> #include <stdexcept> #include <string> #include <utility>src/hybridmap/HybridNeutralAtomMapper.cpp (1)
407-482: Out‑of‑bounds risk inapplyFlyingAncillaindex arithmetic.
applyFlyingAncillausesallCoords[i]and incrementsiby 2 per iteration overfaComb.moves, whileallCoordsis built from the gate’s targets+controls (2 entries for 2‑qubit gates). WhenfaComb.moves.size() > 1(possible ifconvertMoveCombToFlyingAncillaCombproduces entries for both gate qubits),itakes values0, 2, ..., so the second iteration readsallCoords[2]withallCoords.size() == 2, which is out of bounds and leads to UB. This is the same issue called out in an earlier review and remains unresolved.You should either:
- Enforce that
faComb.moves.size() <= 1for 2‑qubit gates (e.g., by truncatingbestFAsto a single entry or asserting the invariant inconvertMoveCombToFlyingAncillaComb), or- Rework
applyFlyingAncillato associate eachFlyingAncillawith an explicit gate coordinate instead of relying onallCoords[i]with a stride of 2 (e.g., store the relevant gate coord inFlyingAncillaand use that directly), eliminating the fragile indexing.Until this is fixed, certain flying‑ancilla move combinations can trigger buffer overruns.
src/hybridmap/MoveToAodConverter.cpp (1)
321-325: Unsigned subtraction can underflow whenend < start.The delta calculation uses unsigned subtraction:
const auto start = static_cast<std::uint32_t>(dim == Dimension::X ? origin.x : origin.y); const auto end = static_cast<std::uint32_t>(dim == Dimension::X ? final.x : final.y); const auto delta = static_cast<qc::fp>(end - start);When
end < start, this underflows before the cast toqc::fp, producing a large positive value instead of the expected negative delta. This was flagged in a past review but appears unaddressed.Apply this fix:
- const auto delta = static_cast<qc::fp>(end - start); + const auto delta = static_cast<qc::fp>( + static_cast<int32_t>(end) - static_cast<int32_t>(start));include/hybridmap/HybridNeutralAtomMapper.hpp (1)
612-640: I/O methods lack error checking despite documented@throwbehavior.The documentation mentions
@throw std::runtime_error On file I/O failure, butsaveMappedQcQasmandsaveMappedQcAodQasmdon't verify stream state after opening or writing. This was flagged in a past review but appears unaddressed.Either add error checking:
void saveMappedQcQasm(const std::string& filename) const { std::ofstream ofs(filename); if (!ofs) { throw std::runtime_error("Failed to open file: " + filename); } mappedQc.dumpOpenQASM(ofs, false); if (!ofs.good()) { throw std::runtime_error("Failed to write to file: " + filename); } }Or remove the
@throwspecification from the documentation.include/hybridmap/NeutralAtomArchitecture.hpp (2)
22-35: Unused includes remain in the file.Per a previous review,
<array>,<cmath>,<iterator>, and<utility>were identified as unused in this header. These includes increase compilation time without providing value.Remove the unused includes:
-#include <array> #include <cassert> -#include <cmath> #include <cstddef> #include <cstdint> #include <fstream> #include <iostream> -#include <iterator> #include <map> #include <set> #include <spdlog/spdlog.h> #include <stdexcept> #include <string> -#include <utility> #include <vector>
573-576: UseOpType::AodMovefor consistency with other shuttling time calculations.This method uses
qc::OpType::Movewhile all other shuttling time queries in the codebase (includinggetAnimationMachine) useqc::OpType::AodMove. The JSON configuration files may define different values for these operation types (e.g.,rubidium_hybrid.jsonhas"move": 0.55vs"aod_move": 0.3), causing this inconsistency to produce incorrect timing estimates.Apply this fix:
[[nodiscard]] qc::fp getVectorShuttlingTime(const MoveVector& v) const { return v.getLength() * getInterQubitDistance() / - getShuttlingTime(qc::OpType::Move); + getShuttlingTime(qc::OpType::AodMove); }Please verify the intended semantics: is
MovevsAodMovea meaningful distinction in your timing model, or should this indeed useAodMovefor consistency?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (23)
bindings/hybrid_mapper/hybrid_mapper.cpp(4 hunks)include/hybridmap/HardwareQubits.hpp(6 hunks)include/hybridmap/HybridAnimation.hpp(1 hunks)include/hybridmap/HybridNeutralAtomMapper.hpp(3 hunks)include/hybridmap/HybridSynthesisMapper.hpp(1 hunks)include/hybridmap/Mapping.hpp(3 hunks)include/hybridmap/MoveToAodConverter.hpp(5 hunks)include/hybridmap/NeutralAtomArchitecture.hpp(4 hunks)include/hybridmap/NeutralAtomDefinitions.hpp(2 hunks)include/hybridmap/NeutralAtomLayer.hpp(2 hunks)include/hybridmap/NeutralAtomScheduler.hpp(3 hunks)include/hybridmap/NeutralAtomUtils.hpp(5 hunks)python/mqt/qmap/hybrid_mapper.pyi(3 hunks)src/hybridmap/HardwareQubits.cpp(7 hunks)src/hybridmap/HybridNeutralAtomMapper.cpp(30 hunks)src/hybridmap/HybridSynthesisMapper.cpp(1 hunks)src/hybridmap/Mapping.cpp(1 hunks)src/hybridmap/MoveToAodConverter.cpp(15 hunks)src/hybridmap/NeutralAtomArchitecture.cpp(8 hunks)src/hybridmap/NeutralAtomLayer.cpp(4 hunks)src/hybridmap/NeutralAtomScheduler.cpp(4 hunks)test/hybridmap/test_hybridmap.cpp(4 hunks)test/python/hybrid_mapper/test_hybrid_synthesis.py(1 hunks)
🧰 Additional context used
🧠 Learnings (14)
📚 Learning: 2025-11-20T18:19:31.857Z
Learnt from: lsschmid
Repo: munich-quantum-toolkit/qmap PR: 832
File: src/hybridmap/HybridNeutralAtomMapper.cpp:1679-1715
Timestamp: 2025-11-20T18:19:31.857Z
Learning: In src/hybridmap/HybridNeutralAtomMapper.cpp, the compareSwapAndBridge and compareShuttlingAndFlyingAncilla methods intentionally use division by zero to produce infinity when distance reductions are zero or negative. This infinity value marks that option as highly unfavorable in cost comparisons, which is the intended behavior rather than a bug.
Applied to files:
src/hybridmap/HybridSynthesisMapper.cppinclude/hybridmap/HybridSynthesisMapper.hppsrc/hybridmap/NeutralAtomArchitecture.cppbindings/hybrid_mapper/hybrid_mapper.cppsrc/hybridmap/Mapping.cppsrc/hybridmap/NeutralAtomLayer.cppinclude/hybridmap/NeutralAtomDefinitions.hppinclude/hybridmap/NeutralAtomScheduler.hppsrc/hybridmap/HardwareQubits.cppsrc/hybridmap/HybridNeutralAtomMapper.cppinclude/hybridmap/Mapping.hppsrc/hybridmap/MoveToAodConverter.cpptest/hybridmap/test_hybridmap.cppsrc/hybridmap/NeutralAtomScheduler.cppinclude/hybridmap/MoveToAodConverter.hppinclude/hybridmap/NeutralAtomArchitecture.hppinclude/hybridmap/HybridNeutralAtomMapper.hppinclude/hybridmap/NeutralAtomUtils.hppinclude/hybridmap/HardwareQubits.hpp
📚 Learning: 2025-11-27T08:52:06.428Z
Learnt from: lsschmid
Repo: munich-quantum-toolkit/qmap PR: 832
File: src/hybridmap/HybridNeutralAtomMapper.cpp:1379-1383
Timestamp: 2025-11-27T08:52:06.428Z
Learning: In src/hybridmap/HybridNeutralAtomMapper.cpp, the getBestMovePos function throws std::runtime_error when no move position is found (finalBestPos.coords is empty). This behavior is acceptable for current use cases since tight architectures are not a concern. A future improvement would be to fall back to SWAP-based strategies instead of throwing.
Applied to files:
src/hybridmap/HybridSynthesisMapper.cppinclude/hybridmap/HybridSynthesisMapper.hppsrc/hybridmap/NeutralAtomArchitecture.cppbindings/hybrid_mapper/hybrid_mapper.cppsrc/hybridmap/Mapping.cppsrc/hybridmap/NeutralAtomLayer.cppinclude/hybridmap/HybridAnimation.hppinclude/hybridmap/NeutralAtomDefinitions.hppinclude/hybridmap/NeutralAtomScheduler.hppsrc/hybridmap/HardwareQubits.cppsrc/hybridmap/HybridNeutralAtomMapper.cppinclude/hybridmap/Mapping.hppsrc/hybridmap/MoveToAodConverter.cpptest/hybridmap/test_hybridmap.cppsrc/hybridmap/NeutralAtomScheduler.cppinclude/hybridmap/NeutralAtomLayer.hppinclude/hybridmap/MoveToAodConverter.hppinclude/hybridmap/NeutralAtomArchitecture.hppinclude/hybridmap/HybridNeutralAtomMapper.hppinclude/hybridmap/NeutralAtomUtils.hppinclude/hybridmap/HardwareQubits.hpp
📚 Learning: 2025-11-20T16:57:24.305Z
Learnt from: lsschmid
Repo: munich-quantum-toolkit/qmap PR: 832
File: test/hybridmap/test_hybridmap.cpp:200-287
Timestamp: 2025-11-20T16:57:24.305Z
Learning: In test/hybridmap/test_hybridmap.cpp, the LongShuttling test is designed as a stress test to catch rare edge cases that only occur after extended shuttling sequences, which is why it maps a long circuit without explicit assertions beyond ensuring no exceptions are thrown.
Applied to files:
src/hybridmap/HybridSynthesisMapper.cpptest/python/hybrid_mapper/test_hybrid_synthesis.pysrc/hybridmap/HybridNeutralAtomMapper.cpptest/hybridmap/test_hybridmap.cppinclude/hybridmap/NeutralAtomUtils.hpp
📚 Learning: 2025-10-07T15:30:42.946Z
Learnt from: MatthiasReumann
Repo: munich-quantum-toolkit/core PR: 1237
File: mlir/include/mlir/Dialect/MQTOpt/Transforms/Transpilation/Layout.h:219-231
Timestamp: 2025-10-07T15:30:42.946Z
Learning: In the Layout class for MLIR quantum routing (mlir/include/mlir/Dialect/MQTOpt/Transforms/Transpilation/Layout.h), the swap method intentionally does NOT swap the hw fields in QubitInfo. This is correct because SSA values represent quantum states at fixed hardware locations, and only their program index associations change during a SWAP gate. The hw field indicates where an SSA value physically resides and remains constant.
Applied to files:
src/hybridmap/HybridSynthesisMapper.cppsrc/hybridmap/NeutralAtomArchitecture.cppsrc/hybridmap/Mapping.cppinclude/hybridmap/NeutralAtomDefinitions.hppsrc/hybridmap/HardwareQubits.cppsrc/hybridmap/HybridNeutralAtomMapper.cppinclude/hybridmap/Mapping.hppinclude/hybridmap/HybridNeutralAtomMapper.hppinclude/hybridmap/HardwareQubits.hpp
📚 Learning: 2025-10-09T04:30:29.071Z
Learnt from: MatthiasReumann
Repo: munich-quantum-toolkit/core PR: 1237
File: mlir/include/mlir/Dialect/MQTOpt/Transforms/Transpilation/Scheduler.h:72-140
Timestamp: 2025-10-09T04:30:29.071Z
Learning: In the CrawlScheduler (mlir/include/mlir/Dialect/MQTOpt/Transforms/Transpilation/Scheduler.h), the persistent `layoutCopy` is updated across all lookahead layers via `remapQubitValue` calls. After each two-qubit gate is scheduled in a layer, its input qubits are remapped to output qubits, ensuring the next layer's crawl starts from those outputs. This progressive remapping through SSA values prevents the same operation from being scheduled in multiple lookahead layers.
Applied to files:
include/hybridmap/HybridSynthesisMapper.hppsrc/hybridmap/NeutralAtomLayer.cppsrc/hybridmap/HybridNeutralAtomMapper.cppsrc/hybridmap/NeutralAtomScheduler.cppinclude/hybridmap/HybridNeutralAtomMapper.hpp
📚 Learning: 2025-12-04T06:59:40.314Z
Learnt from: MatthiasReumann
Repo: munich-quantum-toolkit/core PR: 1301
File: mlir/lib/Dialect/MQTOpt/Transforms/Transpilation/LayeredUnit.cpp:84-85
Timestamp: 2025-12-04T06:59:40.314Z
Learning: In the MQTOpt MLIR routing passes (NaiveRoutingPassSC, AStarRoutingPassSC), the input IR is guaranteed to contain only 1-qubit and 2-qubit gates. All 3+-qubit gates must be decomposed before routing; otherwise the input IR is invalid. This invariant allows skipTwoQubitBlock in LayeredUnit.cpp to safely assert wires.size() == 2.
Applied to files:
src/hybridmap/NeutralAtomArchitecture.cppsrc/hybridmap/Mapping.cppsrc/hybridmap/NeutralAtomLayer.cppsrc/hybridmap/HardwareQubits.cppsrc/hybridmap/HybridNeutralAtomMapper.cppinclude/hybridmap/Mapping.hpptest/hybridmap/test_hybridmap.cppinclude/hybridmap/NeutralAtomLayer.hpp
📚 Learning: 2025-11-01T15:57:31.153Z
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1283
File: src/qir/runtime/QIR.cpp:196-201
Timestamp: 2025-11-01T15:57:31.153Z
Learning: In the QIR runtime (src/qir/runtime/QIR.cpp), the PRX gate (__quantum__qis__prx__body) is an alias for the R gate (Phased X-Rotation) and should call runtime.apply<qc::R>(theta, phi, qubit), not runtime.apply<qc::RX>() which is a single-parameter rotation gate.
Applied to files:
src/hybridmap/NeutralAtomArchitecture.cpptest/hybridmap/test_hybridmap.cpp
📚 Learning: 2025-10-09T13:20:11.483Z
Learnt from: DRovara
Repo: munich-quantum-toolkit/core PR: 1108
File: mlir/test/Dialect/MQTOpt/Transforms/lift-measurements.mlir:269-288
Timestamp: 2025-10-09T13:20:11.483Z
Learning: In the MQT MLIR dialect, the `rz` gate should not be included in the `DIAGONAL_GATES` set for the `ReplaceBasisStateControlsWithIfPattern` because its operator matrix does not have the required shape | 1 0 | / | 0 x | for the targets-as-controls optimization. It is only included in `LiftMeasurementsAboveGatesPatterns` where the matrix structure requirement differs.
Applied to files:
src/hybridmap/NeutralAtomArchitecture.cppsrc/hybridmap/HybridNeutralAtomMapper.cpp
📚 Learning: 2025-10-09T13:13:51.224Z
Learnt from: DRovara
Repo: munich-quantum-toolkit/core PR: 1108
File: mlir/lib/Dialect/MQTOpt/Transforms/ReplaceBasisStateControlsWithIfPattern.cpp:171-180
Timestamp: 2025-10-09T13:13:51.224Z
Learning: In MQT Core MLIR, UnitaryInterface operations guarantee 1-1 correspondence between input and output qubits in the same order. When cloning or modifying unitary operations (e.g., removing controls), this correspondence is maintained by construction, so yielding getAllInQubits() in else-branches matches the result types from the operation's outputs.
Applied to files:
src/hybridmap/NeutralAtomArchitecture.cppsrc/hybridmap/NeutralAtomLayer.cppsrc/hybridmap/HardwareQubits.cppsrc/hybridmap/HybridNeutralAtomMapper.cppinclude/hybridmap/Mapping.hppinclude/hybridmap/HardwareQubits.hpp
📚 Learning: 2025-11-05T09:23:46.540Z
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1287
File: src/qdmi/dd/Device.cpp:492-521
Timestamp: 2025-11-05T09:23:46.540Z
Learning: In the munich-quantum-toolkit/core repository, the `qasm3::Importer::imports()` function is backwards compatible with OpenQASM 2 programs. Therefore, it can be used to parse both QASM2 and QASM3 program formats without requiring separate importers for each version.
Applied to files:
bindings/hybrid_mapper/hybrid_mapper.cpppython/mqt/qmap/hybrid_mapper.pyi
📚 Learning: 2025-12-02T07:37:46.860Z
Learnt from: MatthiasReumann
Repo: munich-quantum-toolkit/core PR: 1301
File: mlir/lib/Dialect/MQTOpt/Transforms/Transpilation/sc/NaiveRoutingPass.cpp:144-151
Timestamp: 2025-12-02T07:37:46.860Z
Learning: In MLIR transformation code (mlir/lib/Dialect/MQTOpt/Transforms/Transpilation/sc/NaiveRoutingPass.cpp and similar), when inserting operations before a target operation, prefer `rewriter.setInsertionPoint(op)` over `rewriter.setInsertionPointAfter(op->getPrevNode())`. The former is cleaner, avoids null pointer edge cases (when op is first in block), and is semantically clearer.
Applied to files:
src/hybridmap/HybridNeutralAtomMapper.cppsrc/hybridmap/MoveToAodConverter.cppinclude/hybridmap/NeutralAtomUtils.hpp
📚 Learning: 2025-10-07T15:58:19.247Z
Learnt from: MatthiasReumann
Repo: munich-quantum-toolkit/core PR: 1237
File: mlir/include/mlir/Dialect/MQTOpt/Transforms/Transpilation/Planner.h:202-214
Timestamp: 2025-10-07T15:58:19.247Z
Learning: In the QMAP routing algorithm (mlir/include/mlir/Dialect/MQTOpt/Transforms/Transpilation/Planner.h), the heuristic function h() intentionally uses a non-admissible heuristic that sums per-gate shortest-path distances. This is a deliberate design choice from the QMAP paper, where admissibility is dropped because locally optimal solutions can negatively affect the overall routing quality. This approach prioritizes global solution quality over guaranteed local optimality.
Applied to files:
src/hybridmap/HybridNeutralAtomMapper.cpp
📚 Learning: 2025-11-05T07:42:45.507Z
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1287
File: test/qdmi/dd/error_handling_test.cpp:118-194
Timestamp: 2025-11-05T07:42:45.507Z
Learning: In the munich-quantum-toolkit/core QDMI device API, session parameters can only be set before calling `device_session_init()`. Once a session is initialized, any attempt to set a parameter returns `QDMI_ERROR_BADSTATE`. Since `SessionGuard` (in test/qdmi/dd/helpers/test_utils.hpp) automatically initializes the session in its constructor, tests that need to verify session parameter setting behavior before initialization must allocate a separate uninitialized session rather than reusing the `SessionGuard`'s session.
Applied to files:
test/hybridmap/test_hybridmap.cppinclude/hybridmap/HybridNeutralAtomMapper.hpp
📚 Learning: 2025-11-24T10:19:41.147Z
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1326
File: python/mqt/core/__init__.py:22-22
Timestamp: 2025-11-24T10:19:41.147Z
Learning: In the munich-quantum-toolkit/core repository, Ruff is configured with 'ALL' rules enabled by default, and only specific rules are selectively disabled. When reviewing changes that enable previously-disabled rules (like PLC0415), noqa directives for those rules become necessary and should be retained.
Applied to files:
include/hybridmap/HybridNeutralAtomMapper.hpp
🧬 Code graph analysis (14)
src/hybridmap/HybridSynthesisMapper.cpp (2)
include/hybridmap/HybridSynthesisMapper.hpp (4)
synthesisSteps(130-131)qc(56-56)qc(137-137)qc(143-143)include/hybridmap/HybridNeutralAtomMapper.hpp (8)
qc(549-556)qc(549-550)qc(558-563)qc(558-559)qc(571-571)qc(678-678)op(134-134)op(385-386)
include/hybridmap/HybridSynthesisMapper.hpp (3)
include/hybridmap/Mapping.hpp (4)
Mapping(61-61)Mapping(66-70)Mapping(66-66)Mapping(81-107)include/hybridmap/NeutralAtomArchitecture.hpp (15)
nodiscard(97-99)nodiscard(103-103)nodiscard(107-107)nodiscard(111-111)nodiscard(115-117)nodiscard(121-123)nodiscard(127-129)nodiscard(133-135)nodiscard(139-139)nodiscard(161-166)nodiscard(231-231)nodiscard(236-238)nodiscard(243-245)maybe_unused(250-252)maybe_unused(591-596)include/hybridmap/NeutralAtomUtils.hpp (2)
maybe_unused(67-81)maybe_unused(90-102)
test/python/hybrid_mapper/test_hybrid_synthesis.py (2)
noxfile.py (1)
qiskit(139-147)python/mqt/qmap/hybrid_mapper.pyi (9)
NeutralAtomHybridArchitecture(50-79)init_mapping(114-114)evaluate_synthesis_steps(107-109)append_with_mapping(103-103)append_without_mapping(104-104)save_mapped_qc_qasm(115-115)convert_to_aod(106-106)get_synthesized_qc(113-113)get_circuit_adjacency_matrix(110-110)
src/hybridmap/NeutralAtomLayer.cpp (1)
include/hybridmap/NeutralAtomLayer.hpp (5)
qubitsToUpdate(57-57)qubitsToUpdate(67-67)qubitsToUpdate(76-76)commutesWithAtQubit(138-139)commuteAtQubit(150-151)
include/hybridmap/HybridAnimation.hpp (1)
src/hybridmap/HybridAnimation.cpp (2)
initPositions(30-57)initPositions(30-32)
include/hybridmap/NeutralAtomScheduler.hpp (2)
include/hybridmap/NeutralAtomArchitecture.hpp (14)
nodiscard(97-99)nodiscard(103-103)nodiscard(107-107)nodiscard(111-111)nodiscard(115-117)nodiscard(121-123)nodiscard(127-129)nodiscard(133-135)nodiscard(139-139)nodiscard(161-166)nodiscard(231-231)nodiscard(236-238)nodiscard(243-245)shuttlingSpeedFactor(584-584)src/hybridmap/NeutralAtomArchitecture.cpp (2)
getAnimationMachine(265-319)getAnimationMachine(265-266)
src/hybridmap/HardwareQubits.cpp (1)
include/hybridmap/HardwareQubits.hpp (11)
q1(78-78)q1(154-154)q1(281-282)q(300-300)q(309-309)hwQubit(179-179)hwQubit(187-209)hwQubit(187-187)idx(167-167)idx(317-317)idx(325-325)
src/hybridmap/HybridNeutralAtomMapper.cpp (3)
include/hybridmap/HardwareQubits.hpp (11)
op(215-220)op(215-215)q(300-300)q(309-309)qubit(70-70)qubit(228-228)qubits(332-332)qubits(363-363)q1(78-78)q1(154-154)q1(281-282)include/hybridmap/Mapping.hpp (10)
op(199-204)op(199-199)qubit(113-115)qubit(113-113)qubit(124-124)qubit(170-170)qubit(187-187)qubits(136-136)qubits(152-152)swap(213-213)src/hybridmap/Mapping.cpp (2)
applySwap(27-42)applySwap(27-27)
python/mqt/qmap/hybrid_mapper.pyi (3)
python/mqt/qmap/sc.pyi (3)
name(109-109)name(173-173)map(258-270)test/hybridmap/test_hybridmap.cpp (1)
params(79-88)bindings/sc/sc.cpp (2)
map(50-78)map(51-52)
test/hybridmap/test_hybridmap.cpp (2)
include/hybridmap/NeutralAtomArchitecture.hpp (1)
NeutralAtomArchitecture(216-216)include/hybridmap/HybridNeutralAtomMapper.hpp (7)
NeutralAtomMapper(447-447)NeutralAtomMapper(448-469)NeutralAtomMapper(448-449)NeutralAtomMapper(470-472)NeutralAtomMapper(470-471)p(480-493)p(480-480)
src/hybridmap/NeutralAtomScheduler.cpp (2)
include/hybridmap/HybridAnimation.hpp (2)
initHwPos(53-54)op(94-95)include/hybridmap/NeutralAtomArchitecture.hpp (5)
shuttlingSpeedFactor(584-584)op(329-329)op(335-335)op(342-342)c(418-418)
include/hybridmap/NeutralAtomArchitecture.hpp (1)
src/hybridmap/NeutralAtomArchitecture.cpp (2)
getAnimationMachine(265-319)getAnimationMachine(265-266)
include/hybridmap/HybridNeutralAtomMapper.hpp (3)
include/hybridmap/HardwareQubits.hpp (5)
op(215-220)op(215-215)HardwareQubits(88-88)HardwareQubits(100-143)HardwareQubits(100-103)include/hybridmap/Mapping.hpp (6)
op(199-204)op(199-199)Mapping(61-61)Mapping(66-70)Mapping(66-66)Mapping(81-107)test/hybridmap/test_hybridmap.cpp (5)
mapper(94-94)mapper(227-227)mapper(263-263)mapper(272-272)mapper(285-285)
include/hybridmap/NeutralAtomUtils.hpp (1)
src/hybridmap/NeutralAtomUtils.cpp (4)
computeBridgeCircuit(132-148)computeBridgeCircuit(132-132)computeGates(110-130)computeGates(110-110)
🪛 GitHub Check: 🇨 Lint / 🚨 Lint
src/hybridmap/HybridNeutralAtomMapper.cpp
[warning] 32-32: src/hybridmap/HybridNeutralAtomMapper.cpp:32:1 [misc-include-cleaner]
included header iostream is not used directly
[warning] 31-31: src/hybridmap/HybridNeutralAtomMapper.cpp:31:1 [misc-include-cleaner]
included header iomanip is not used directly
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: 🐍 Test (macos-15-intel) / 🐍 macos-15-intel
- GitHub Check: 🐍 Test (windows-2022) / 🐍 windows-2022
🔇 Additional comments (79)
src/hybridmap/NeutralAtomArchitecture.cpp (5)
76-91: Fallback gate handling looks good.The lambda
ensureGateWithFallbackproperly validates that required gates (cz,h) exist in the timing/fidelity maps, falling back to"none"if available or throwing a clear error otherwise. This ensures the downstream SWAP and Bridge calculations won't fail due to missing entries.
115-132: Bridge gate precomputation bounds check properly implemented.The bounds check using
maxIdx = std::min({...})andstd::min<std::size_t>(10, maxIdx)addresses the previous concern about unconditional indexing. This ensures safe access tobridgeCircuitsarrays.
265-271: Positive guard for shuttlingSpeedFactor is correct.The validation throws with a clear message if
shuttlingSpeedFactor <= 0, preventing division-by-zero and nonsensical negative speed factors.
334-348: Phase normalization and loop bounds are correct.The iteration
for (size_t i = 1; i < nQubits; ++i)safely handles thenQubits == 0case, and thefmod-based phase normalization is clearer and more efficient than the previous loop.
376-392: Ancilla qubit blocking logic is correct.The while loop properly maps ancilla qubits (which may be indexed beyond
getNpositions()) back to their primary coordinate positions. Adding blocked indices at offsetsi + getNpositions()andi + 2*getNpositions()correctly reserves space in the ancilla zones for the flying ancilla support mentioned in the PR objectives.python/mqt/qmap/hybrid_mapper.pyi (3)
24-48: MapperParameters and MapperStats type definitions are well-structured.The parameter and stats classes expose clear, typed attributes. The naming follows Python conventions (snake_case), and the types are appropriate for the data they represent.
81-98: HybridNAMapper API surface is well-defined.The method naming clearly distinguishes QASM string returns (
get_mapped_qc_qasm,get_mapped_qc_aod_qasm) from other accessors. Theschedulemethod's parameter types (typing.SupportsFloatforshuttling_speed_factor) allow flexible numeric input from Python.
100-121: HybridSynthesisMapper stubs align with C++ API.The synthesis mapper exposes a comprehensive API for synthesis workflow: evaluation (
evaluate_synthesis_steps), appending (append_with_mapping,append_without_mapping), remapping (complete_remap), and I/O (save_*,get_*). Return types are consistent withHybridNAMapper.test/python/hybrid_mapper/test_hybrid_synthesis.py (3)
25-34: Module-level circuit creation is acceptable for test setup.Creating
qc1andqc2at module level viaload(qc_qiskit)is a common pattern for test fixtures. Since these are simple 3-qubit circuits used across multiple tests, this avoids redundant construction overhead.
70-96: Test properly uses tmp_path for file artifacts.The test correctly uses pytest's
tmp_pathfixture for all output files, ensuring automatic cleanup and avoiding pollution of the source tree.
127-131: Keep-alive test validates object lifetime correctly.This test ensures the
HybridSynthesisMappercreated via a helper function remains valid after the helper returns. The lack of explicit assertions is intentional—the test passes if no crash occurs, verifying correct object ownership across the Python/C++ boundary.src/hybridmap/Mapping.cpp (2)
27-42: applySwap correctly uses const reference.The signature change to
const Swap& swapand the use of local const variables (q1,q2) improves const-correctness and avoids unnecessary copies.
117-187: Graph matching algorithm is correctly implemented.The algorithm uses a greedy approach: starting from the highest-degree circuit qubit mapped to the most-connected hardware qubit, it propagates mappings by minimizing weighted distances to already-mapped neighbors. The implementation correctly handles edge cases (empty queue, no free hardware qubits) with appropriate exceptions.
src/hybridmap/HybridSynthesisMapper.cpp (4)
31-68: evaluateSynthesisSteps correctly handles edge cases.The implementation properly addresses previous concerns:
- Empty input guard returns early (lines 34-36)
- Evaluation order is deterministic via separate
fidelityvariable (line 47)alsoMapguard checks!candidates.empty()(line 64)- Logging uses consistent
++qcIndexat loop end (line 52)
70-80: evaluateSynthesisStep correctly preserves input.Making a copy (
auto qcCopy = qc) before callingtempMapper.map(qcCopy, mapping)ensures the original circuit is not mutated, addressing the previous const-correctness concern.
93-101: Verify whethersynthesizedQcshould contain mapped or unmapped operations.
appendWithMappingcallsmapAppend(qc, mapping)which mutatesqc(MCX→MCZ conversion, fusion, etc.), then clones the resulting operations intosynthesizedQc. However,synthesizedQcis described as the "unmapped" circuit in the header.If
synthesizedQcis intended to hold the original unmapped circuit, you should cloneqcbefore callingmapAppend:void HybridSynthesisMapper::appendWithMapping(qc::QuantumComputation& qc) { if (mappedQc.empty()) { initMapping(qc.getNqubits()); } + // Clone original ops before mapping mutates qc + for (const auto& op : qc) { + synthesizedQc.emplace_back(op->clone()); + } mapAppend(qc, mapping); - for (const auto& op : qc) { - synthesizedQc.emplace_back(op->clone()); - } }Please verify the intended semantics of
synthesizedQc.
103-129: Adjacency matrix construction correctly fills symmetric entries.The implementation now explicitly sets both
adjMatrix(i, j)andadjMatrix(j, i)for each pair, ensuring the matrix is symmetric regardless ofAdjacencyMatrix's internal storage model.include/hybridmap/HybridSynthesisMapper.hpp (3)
66-69: Constructor correctly delegates to base class.The explicit constructor properly initializes the
NeutralAtomMapperbase class with architecture and parameters.
91-94: completeRemap correctly preserves synthesizedQc.Making a local copy before calling
map()ensures the canonical unmapped circuit insynthesizedQcis not mutated, allowingcompleteRemap()to be called multiple times with different initial mappings.
130-149: Public API methods handle initialization gracefully.Methods like
evaluateSynthesisStepsandappendWith*implicitly initialize if needed (checking!initializedormappedQc.empty()), whilegetCircuitAdjacencyMatrixthrows if not initialized. This provides flexibility while preventing misuse.src/hybridmap/NeutralAtomLayer.cpp (3)
42-68: Front-layer vs. lookahead branching logic is clear and well-structured.The two code paths are cleanly separated:
- Front-layer mode advances while operations commute with existing gates and candidates
- Lookahead mode collects up to
lookaheadDepthmulti-qubit gates per qubitThe logic correctly stops advancing on the first non-commuting operation in front-layer mode.
129-135: LGTM! Free function for layer-wide commutation check.The
commutesWithAtQubitfree function properly usesstd::ranges::all_ofto check commutation against all operations in a layer.
161-176: The current code usingstd::ranges::findfor target membership checks is correct and cannot be changed tocontains().getControls()andgetTargets()return different container types—controls is a set-like container with acontains()method, while targets is a vector-like container without one. Usingstd::ranges::findon targets is the appropriate idiom for vector membership checks.Likely an incorrect or invalid review comment.
include/hybridmap/HybridAnimation.hpp (2)
24-33: Good documentation of lifetime contract.The class docstring now explicitly documents that "The architecture reference passed to the constructor must remain valid for the lifetime of this object." This addresses the previous feedback about documenting the non-owning pointer lifetime requirement.
71-95: Well-documentedopToNaVizcontract.The documentation now explicitly covers supported operation types (AOD load/store/move, multi-qubit gates, single-qubit gates) and states that other operations are not currently supported. This addresses prior feedback about clarifying the operation handling contract.
include/hybridmap/NeutralAtomDefinitions.hpp (1)
79-110:AtomMovestruct with comparison operators looks good.The struct now includes
operator<usingstd::tiefor lexicographic ordering, addressing the previous feedback. This enables use in ordered containers likestd::setandstd::map.include/hybridmap/NeutralAtomLayer.hpp (1)
138-151: LGTM! Well-documented free functions for commutation checks.The
commutesWithAtQubitandcommuteAtQubitfree functions are clearly documented with their commutation rules, including handling of non-unitaries, identities, single-qubit gates, and two-qubit control/target patterns.include/hybridmap/Mapping.hpp (2)
81-92: Good: Added validation fornQubitsparameter.The constructor now validates that
nQubitsdoes not exceedqc.getNqubits(), addressing the previous concern about potential out-of-bounds access in theGraphbranch. The parameter is also now passed by reference, avoiding an unnecessary copy.
144-159: LGTM! New vector-basedgetHwQubitsoverload.The new overload preserves input order, which is useful when the caller needs ordered results. Good use of
reserve()to avoid reallocations.include/hybridmap/NeutralAtomScheduler.hpp (2)
37-95: SchedulerResults and arch pointer usage look sound.The extended
SchedulerResults(AOD counters, ctor,toMap) and the switch from a reference member to a non-owningconst NeutralAtomArchitecture*with a deleted default constructor cleanly address earlier UB risks around uninitialized architectures. The API surface (ctor +schedulereturn type) is consistent and backwards-compatible at the call sites.
142-183: Guarded animation saving avoids empty artifacts; behavior is consistent.
saveAnimationFilesnow checksanimation.empty() || animationMachine.empty()and logs a warning instead of blindly writing files, and the docstring matches the actual.naviz/.namachineoutputs. This prevents confusing empty files whenschedule(..., createAnimationCsv=true, ...)was not called, while intentionally requiring both artifacts to be present before writing.src/hybridmap/HardwareQubits.cpp (2)
91-135: Shortest‑paths computation for bridges is correct and self‑limiting.
computeAllShortestPathscorrectly uses BFS overgetNearbyQubitsto enumerate all simple paths of minimal length between two qubits and stops exploring longer paths once a shortest length is known. This is logically sound for bridge selection; the potential combinatorial blowup is acceptable in this narrow use (front‑layer gates only).
137-180: move() + getClosestQubit now maintain a consistent, safe hardware view.The new
moveimplementation keepsoccupiedCoordinates,freeCoordinates,nearbyQubits, andhwToCoordIdxin sync and validates coordinates againstarch->getNpositions(). Together withresetSwapDistances(), this ensures swap distances reflect the updated layout.
getClosestQubitno longer returns0as a silent default when no non‑ignored qubit exists; instead it tracks whether any candidate was found and throws astd::runtime_errorif not. This resolves the earlier latent correctness bug where an invalid qubit could be selected and used downstream.Also applies to: 297-318
src/hybridmap/HybridNeutralAtomMapper.cpp (5)
47-125: mapAppend and initial layering flow are consistent with new features.The
mapAppendpipeline (pre‑optimization, multi‑qubit‑gate detection, ancilla/flying‑ancilla register provisioning, DAG construction, initial gate mapping, and the front/lookahead layer loop) is structurally sound. ReusingmappedQcwith conditional ancilla-register creation cleanly supports incremental mapping, and themultiQubitGatesflag correctly disables bridge/flying‑ancilla paths when >2‑qubit ops are detected.Also applies to: 127-138
187-235: Pass‑by application keeps logical↔hardware state coherent.
applyPassByupdates only the mapped circuit (mappedQc.move) and the per‑gate coordinate lists, leavingmappingandhardwareQubitsuntouched. That matches the intended semantics of pass‑by being an AOD‑level maneuver only. The use of the ancilla register[npositions, 2*npositions)and the restoration moves at the end keeps the qubit layout consistent.
548-638: Bridge selection and coordinate usage tracking now look robust.
findBestBridge+getShortestBridgescorrectly constrain bridges bymaxBridgeDistance, use all shortest paths fromHardwareQubits::computeAllShortestPaths, and break ties by summing usage counts fromcomputeCurrentCoordUsages(). The latter now sizescoordUsagesas3 * arch->getNpositions()and accounts for front‑layer gates, recently mapped gates, and the latest blocked‑qubit set, which resolves the earlier risk of indexing beyond the logical register space.
639-703: Flying‑ancilla / pass‑by conversion logic matches intended semantics.
convertMoveCombToFlyingAncillaCombandconvertMoveCombToPassByCombcorrectly filter moves to those touching the gate’s coordinates, respect the current set of flying ancillas and theusePassByflag, and construct combinations that align with the later application routines (moving via FA register vs. pass‑by ancilla register). Aside from the indexing issue inapplyFlyingAncillaitself, the construction of these combs is coherent.
705-736: Swap and move distance reduction helpers are consistent with cost model.
swapDistanceReductionandmoveCombDistanceReductionboth compute total distance reduction per layer by comparing pre‑ and post‑movegetAllToAllSwapDistance/getAllToAllEuclideanDistancevalues, mutating a local copy of the qubit set/coord indices. Skipping swaps that touch both endpoints of a gate is intentional and keeps the model simple. The new helpers nicely factor out logic used byswapCostand the dynamic mapping heuristics.Also applies to: 737-786
src/hybridmap/NeutralAtomScheduler.cpp (1)
34-66: Scheduler refactor and metric extensions look correct.The reworked
schedulecorrectly sizes coord‑indexed arrays as3 * arch->getNpositions(), usesarch->getOpTime()/getOpFidelity()andgetBlockedCoordIndices()for timing and Rydberg‑blocking, and tracksnCZs,nAodActivate, andnAodMovealongside idle and decoherence effects. The optionalAnimationAtomsplusgetAnimationMachine(shuttlingSpeedFactor)integrate cleanly with the new animation strings, andprintSchedulerResultsmirrors the extended metrics.Also applies to: 78-181
bindings/hybrid_mapper/hybrid_mapper.cpp (3)
44-95: MapperParameters and MapperStats bindings are well‑structured.The expanded
MapperParametersand newMapperStatsPython bindings cleanly expose all relevant tuning knobs and counters with clear, accurate docstrings. The default constructor plusdef_readwritefields match the C++ layout and make the parameter space discoverable from Python.
109-224: HybridNAMapper binding now has correct lifetimes and clearer I/O.The
HybridNAMapperctor andset_parametersnow usepy::keep_aliveto match the mapper’s non‑owning pointers toNeutralAtomArchitectureandMapperParameters, eliminating prior dangling‑pointer risks from temporaries. The mapping/scheduling interface (map,map_qasm_file,get_stats,get_mapped_qc_qasm,get_mapped_qc_aod_qasm,save_mapped_qc_aod_qasm,schedule) is coherent, and the animation bindings correctly describe.naviz/.namachineoutputs and expose the.navizcontent directly.
226-321: HybridSynthesisMapper Python API is comprehensive and consistent.The new
HybridSynthesisMapperbindings mirror the C++ API, including parameter lifetime management, synthesized/mapped QASM getters/savers, incremental append (with/without mapping), adjacency export, synthesis‑step evaluation, complete remap, and scheduling. The adjacency‑matrix lambda now pre‑sizes thestd::vector<std::vector<int>>and usessize_twith proper includes, and the docstrings clearly distinguish circuit‑level adjacency from hardware connectivity. Overall, this provides a well‑designed Python surface for the hybrid synthesis workflow.include/hybridmap/HardwareQubits.hpp (4)
44-52: LGTM: Protected member additions are appropriate.The new protected members (
nQubits,freeCoordinates,occupiedCoordinates) properly support the expanded functionality. Thearchpointer defaulting tonullptris acceptable given the explicit constructor requirement.
100-143: Constructor logic is well-structured.The initialization correctly handles both trivial and random mappings, populating
occupiedCoordinatesduring assignment and derivingfreeCoordinatesafterward. Theassertfor bounds checking is acceptable for internal library use.One minor note: the
assertonly fires in debug builds. If this class could be constructed with invalid parameters in production, consider a runtime check.
187-209: Correctly fixedremoveHwQubitimplementation.The implementation now properly:
- Captures the current coordinate before erasing mappings.
- Removes the coordinate from
occupiedCoordinates.- Adds it to
freeCoordinatesonly if not already present.- Invalidates swap distances and cleans up nearby qubit references.
This addresses the critical issues identified in past reviews.
167-170: Linear search inisMappedis acceptable for typical architectures.Using
std::ranges::findon a vector is O(n), but given neutral atom architectures typically have a limited number of positions, this should not be a performance concern. If architectures grow significantly larger, consider usingstd::setforoccupiedCoordinates.test/hybridmap/test_hybridmap.cpp (4)
91-122: Parameterized mapping test provides good coverage.The
MapCircuitsIdentitytest exercises the mapper with various architecture, circuit, and parameter combinations. The assertions onqcMapped.size(),scheduleResults.totalFidelities,totalIdleTime, andtotalExecutionTimeprovide reasonable coverage.
167-216: Output test comprehensively covers I/O and statistics.The test properly:
- Uses temporary directory for output files.
- Verifies file existence and non-zero size.
- Cleans up files after assertions.
- Checks various output methods (
getMappedQcQasm,getMappedQcAodQasm,getStats,getStatsMap,getAnimationViz).This addresses previous review concerns about temp file handling.
220-277: Exception tests properly validate error conditions.The tests correctly verify:
- Circuit size exceeding hardware capacity throws.
- Multiple flying ancillas (>1) throws at construction.
- Shuttling weight > 0 without free coordinates throws.
- Unmappable multi-qubit gates throw.
EXPECT_THROWwith explicitstd::runtime_errortyping is appropriate.
279-290: LongShuttling stress test intentionally lacks assertions.Per previous discussion, this test is designed to catch rare edge cases during extended shuttling sequences. The lack of explicit assertions is intentional—the test passes if no exception is thrown. Based on learnings from past review.
Consider adding a brief comment like
// Stress test: exercises rare edge cases in extended shuttling sequencesfor future maintainers.include/hybridmap/NeutralAtomUtils.hpp (5)
60-102: Parsing functions now have clear error messages.The error messages now include the expected values (e.g.,
"trivial"/"0"or"random"/"1"), addressing past review feedback. The case-sensitive behavior is appropriately documented.
110-212: Direction and MoveVector structs are well-designed.Both structs have clear semantics, appropriate const-correctness, and comprehensive documentation. The
MoveVectormethods (sameDirection,getLength,overlap,include) provide useful geometric predicates for move analysis.
235-252: FlyingAncillaComb and PassByComb now have safe default initialization.The
oppointers now default tonullptr, addressing the past review concern about uninitialized pointers. This prevents accidental use of indeterminate values.
259-325: MoveComb implementation is correct.Key design choices:
- Equality comparison intentionally ignores
cost,op, andbestPosto focus on move sequence identity.appendmethods resetcosttomax()to force recomputation.empty()andsize()delegate correctly to the underlying vector.
406-433: BridgeCircuits precomputation is appropriate for expected use.The eager precomputation of bridge circuits for lengths
[3, maxLength)is acceptable given the author's confirmation thatmaxLengthis typically below 5 in practice. For larger values, lazy initialization could be considered, but current design is sufficient.include/hybridmap/HybridNeutralAtomMapper.hpp (7)
44-60: MapperParameters is well-structured with sensible defaults.The parameter struct provides clear configuration options with reasonable default values. The
limitShuttlingLayer = 10default is documented here; ensure Python binding docstrings match if they claim "unlimited" (per past review feedback on bindings file).
62-71: MapperStats provides useful runtime counters.The counters for different routing strategies (
nSwaps,nBridges,nFAncillas,nMoves,nPassBy) enable performance analysis and debugging.
447-472: Constructor correctly stores parameters by value, fixing past lifetime issues.The critical issues from past reviews are resolved:
MapperParametersis now stored by value (parametersmember), not as a pointer.- Signed arithmetic (
static_cast<int>) prevents unsigned underflow in the free-coords check.- Both constructors validate constraints (shuttling/flying ancillas) before proceeding.
The delegation from the reference-based constructor to the pointer-based one is safe since
&architectureyields a stable pointer.
480-493:setParametersnow has consistent validation with constructor.The method:
- Stores parameters by value (safe).
- Validates the shuttling constraint using signed arithmetic.
- Validates
numFlyingAncillas <= 1(addressing past review that this check was missing).- Resets internal state after parameter update.
500-510:copyStateFromnow includesflyingAncillasper past review.The method correctly copies all relevant state including
flyingAncillas, addressing the previous omission that could have caused stale ancilla state issues.
558-563: Unused DAG construction removed per past review.The
map(qc, InitialMapping)overload now directly constructs theMappingwithout the redundant DAG computation that was flagged in past reviews.
594-596:getMappedQcnow returns const reference per past review.Returning a const reference avoids the expensive deep copy that was previously performed when returning by value.
src/hybridmap/MoveToAodConverter.cpp (5)
69-81:convertOpToMovecorrectly handles multi-region qubit indices.The method extracts load flags before normalizing qubit indices, ensuring correct interpretation of flying ancilla vs. data qubit moves. The while loops safely reduce indices to the valid range.
82-116:initFlyingAncillascorrectly initializes flying ancilla positions.The method properly:
- Early returns if no ancillas are configured.
- Computes coordinate positions including offsets based on architecture parameters.
- Emits a single
AodActivateoperation to initialize all ancillas.
537-549: Iterator safety issue fixed per past review.The target qubit ordering logic now safely inserts at
posinstead ofpos - 1, avoiding undefined behavior whenpos == targetQubits.begin(). The fix maintains the intended "move away before used" ordering.
437-493:processMovesandprocessMovesFahave correct control flow.The methods properly:
- Check activation compatibility for both X and Y dimensions.
- Fall back to creating a new move group when activation is impossible.
- Separate flying ancilla moves (
processMovesFa) from regular moves.Note: These methods rely on
canAddActivation, which has the unsigned underflow issue noted in the separate comment.
666-726: AOD operation generation handles activation/deactivation symmetry correctly.The implementation:
- Builds operations only for moves with the appropriate load flag.
- Returns empty vector when no operations needed (lines 704-706).
- Reverses operation order for deactivation to maintain correct sequence.
include/hybridmap/MoveToAodConverter.hpp (5)
13-29: LGTM!The include list is clean and appropriately organized. The removal of
<sys/types.h>and use of<cstdint>instead addresses the previous portability concerns.
31-41: LGTM!The
ActivationMergeTypeenum andMergeTypeXYalias are well-designed and properly documented. The enum class provides type safety, and the pair abstraction for per-dimension merge types is clean.
51-67: LGTM!The
AncillaAtomstruct properly usesstd::uint32_tfor portable fixed-width integers, addressing the previous review feedback. The design cleanly separates coordinate and offset tracking for both original and dodged positions.
135-148: LGTM!The
AodActivationHelperconstructor correctly stores the architecture as a pointer and properly deletes copy/move constructors. The pointer storage is appropriate for a helper class whose lifetime is managed by the parentMoveToAodConverter.
287-296: LGTM!The assertion addresses the previous review concern about potential UB when both containers are empty. The logic correctly handles all three cases: only
movespopulated, onlymovesFapopulated, or both populated.include/hybridmap/NeutralAtomArchitecture.hpp (7)
82-92: LGTM!The
Propertiesconstructor properly validatesaodDist > 0before computingnAodIntermediateLevels, addressing the previous concern about potential division by zero ingetOffsetDistance(). The assertion thatnAodIntermediateLevels >= 1provides additional safety.
161-166: LGTM!The
tEff()implementation correctly handles edge cases. When both T1 and T2 are zero, it returns 0 to avoid division by zero. When only one is zero, the formula naturally returns 0, which is physically correct (if either decoherence mechanism is infinitely fast, effective time is zero).
184-185: LGTM!The comment now correctly documents that bridge circuits are computed for distances 3-9, which matches the
BridgeCircuitsconstructor logic (iterates from 3 tomaxLength-1= 9).
353-376: LGTM!The diagnostic output now uses
SPDLOG_WARNinstead ofstd::cout, addressing the previous review feedback about avoiding console output in library code. The fallback behavior and potential exception from missing "none" entry are properly documented.
427-431: LGTM!The assertion at line 429 addresses the previous review concern about potential out-of-bounds access. This provides debug-time safety while keeping release builds lean.
462-503: LGTM!The new distance helper methods (
getMoveCombEuclideanDistance,getFaEuclideanDistance,getPassByEuclideanDistance) provide clean abstractions for computing path lengths of different move combinations. The formulas correctly account for the geometric characteristics of each move type.
583-596: LGTM!The animation export methods are well-designed. The
getAnimationMachineimplementation (per the relevant code snippet) properly validates thatshuttlingSpeedFactor > 0and throws a clear error otherwise. The file-writing wrapper is appropriately simple.
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/hybridmap/NeutralAtomLayer.cpp (1)
129-178: Commutation logic is comprehensive and correct.The free functions implement reasonable commutation rules:
- Non-unitaries never commute (line 139-141)
- Single-qubit gates always commute with each other (lines 143-145)
- Identity gates commute with everything (lines 147-149)
- Operations not using the qubit commute (lines 152-156)
- Two controlled gates commute at control qubit (lines 161-164)
- Control and Z commute (lines 166-169)
- Same gate type on same target commutes (lines 172-176)
Minor note: Line 172-174 uses
std::ranges::findongetTargets()which returns a container that likely has acontainsmethod likegetControls()does at lines 161-162, 166-167. Consider usingcontainsfor consistency if available.- if (std::ranges::find(op1->getTargets(), qubit) != op1->getTargets().end() && - std::ranges::find(op2->getTargets(), qubit) != op2->getTargets().end() && + // If getTargets() supports contains(): + if (op1->getTargets().contains(qubit) && + op2->getTargets().contains(qubit) && op1->getType() == op2->getType()) {src/hybridmap/HardwareQubits.cpp (1)
224-242: ReplacehwToCoordIdx.maxKey()withnQubitsto include all hardware qubitsThe loop condition uses
hwToCoordIdx.maxKey()which excludes the last qubit from blocking analysis. Given the initialization pattern where keys range from 0 to nQubits-1, the loop should befor (uint32_t i = 0; i < nQubits; ++i)to match patterns used elsewhere (e.g., initTrivialSwapDistances).
♻️ Duplicate comments (16)
test/python/hybrid_mapper/test_hybrid_synthesis.py (2)
56-60: Extend fidelity bounds assertions to both elements.The test checks bounds for
best_circuit[0]but notbest_circuit[1]. Since both are fidelity values, both should be validated.assert best_circuit[0] <= 1 assert best_circuit[0] >= 0 + assert best_circuit[1] <= 1 + assert best_circuit[1] >= 0
109-111: Simplify symmetry check with NumPy.The nested loop can be replaced with a single NumPy assertion for clarity.
- for i in range(circ_size): - for j in range(circ_size): - assert adj_mat[i, j] == adj_mat[j, i] + assert np.array_equal(adj_mat, adj_mat.T), "Adjacency matrix should be symmetric"include/hybridmap/HybridSynthesisMapper.hpp (2)
118-122:saveSynthesizedQcdoesn't throw on file errors despite documentation.The implementation opens a file and writes without enabling exceptions or checking stream state. If the file cannot be opened or written, errors will be silently ignored.
Either enable exceptions to match expected throwing behavior:
[[maybe_unused]] void saveSynthesizedQc(const std::string& filename) const { std::ofstream ofs(filename); + ofs.exceptions(std::ios::failbit | std::ios::badbit); synthesizedQc.dumpOpenQASM(ofs, false); ofs.close(); }Or add explicit error checking:
[[maybe_unused]] void saveSynthesizedQc(const std::string& filename) const { std::ofstream ofs(filename); + if (!ofs) { + throw std::ios_base::failure("Failed to open file: " + filename); + } synthesizedQc.dumpOpenQASM(ofs, false); ofs.close(); }
77-85: Consider documenting single-use semantics or adding reset capability.
initMappingreinitializesmappedQc,synthesizedQc, andmappingbut doesn't reset inherited base-class state (e.g., stats,mappedQcAOD). If instances are reused across multiple synthesis runs, stale state could leak.Either:
- Document that
HybridSynthesisMapperis intended for single-run use, or- Add a base-class reset call in
initMappingsrc/hybridmap/HybridSynthesisMapper.cpp (1)
93-101:appendWithMappingmutates its input circuit.The function takes
qcby non-const reference and passes it tomapAppend, which runs optimization passes that modify the circuit. This may surprise callers who expect evaluation-like behavior.Consider either:
- Taking
const qc::QuantumComputation&and working on a local copy, or- Documenting that the input may be modified
-void HybridSynthesisMapper::appendWithMapping(qc::QuantumComputation& qc) { +void HybridSynthesisMapper::appendWithMapping(const qc::QuantumComputation& qc) { + auto localQc = qc; if (mappedQc.empty()) { - initMapping(qc.getNqubits()); + initMapping(localQc.getNqubits()); } - mapAppend(qc, mapping); - for (const auto& op : qc) { + mapAppend(localQc, mapping); + for (const auto& op : localQc) { synthesizedQc.emplace_back(op->clone()); } }src/hybridmap/NeutralAtomLayer.cpp (1)
31-37:initAllQubitsassumesdag.size()equals number of qubits.This builds
allQubitsas0..dag.size()-1. This is correct only ifdagis indexed by logical qubit anddag.size()matches the qubit count. Consider documenting this invariant or using an explicit qubit count source.include/hybridmap/NeutralAtomDefinitions.hpp (1)
43-54: HwPositions deprecation could use[[deprecated]]attribute.
HwPositionsis marked[[maybe_unused]]with a comment indicating deprecation. For stronger compiler enforcement,[[deprecated("...")]]would generate warnings when used.include/hybridmap/NeutralAtomLayer.hpp (1)
87-99: Constructor is well-designed; consider reservingendsvector.The constructor properly:
- Moves the DAG to avoid copies
- Initializes mode and depth parameters
- Reserves space for iterators and candidates
Minor:
endsis not reserved likeiteratorsandcandidates. For consistency and to avoid small reallocations:iterators.reserve(dag.size()); candidates.reserve(dag.size()); + ends.reserve(dag.size()); for (auto& i : dag) {src/hybridmap/NeutralAtomScheduler.cpp (1)
34-64: Still no global validation ofshuttlingSpeedFactorfor non‑animation runsThe schedule path multiplies AOD op times by
shuttlingSpeedFactorunconditionally, butgetAnimationMachineonly enforces a positive factor whencreateAnimationCsvis true. For robustness, it would still be valuable to validateshuttlingSpeedFactor > 0at the start ofschedule()and throw a clear error otherwise, so non‑animation use cannot silently produce zero or negative durations.src/hybridmap/HybridNeutralAtomMapper.cpp (2)
405-474: Out-of-bounds risk inapplyFlyingAncillawhen multiple flying-ancilla moves exist
applyFlyingAncillauses a sharedallCoordsvector and an indexiadvanced by 2 per move:auto allCoords = targetCoords; allCoords.insert(allCoords.end(), controlCoords.begin(), controlCoords.end()); uint32_t i = 0; for (const auto& passBy : faComb.moves) { ... mappedQc.cz(allCoords[i], ancQ1); ... i += 2; } ... i = 0; for (const auto& passBy : faComb.moves) { ... mappedQc.cz(allCoords[i], ancQ1); i += 2; }For 2‑qubit gates,
allCoords.size()is 2. IffaComb.moves.size()ever becomes ≥ 2 (e.g., both gate qubits generate flying‑ancilla moves inconvertMoveCombToFlyingAncillaComb), the second iteration accessesallCoords[2], which is out of range.Even if current heuristics typically produce at most one flying‑ancilla move per gate, this assumption is not enforced here and any future change to move generation could trigger undefined behavior.
Consider adding a hard guard before the loops:
const auto allCoords = /* as now */; if (faComb.moves.size() * 2 > allCoords.size()) { throw std::runtime_error( "Invalid flying-ancilla combination: too many moves for available coords"); }or otherwise restructuring the indexing so each
FlyingAncillaexplicitly carries the data‑qubit coordinate it couples to instead of relying on thisi += 2pattern.
185-233: Pass-by implementation is consistent; consider avoiding the extra clone
applyPassBycorrectly:
- maps op targets/controls through
MappingandHardwareQubitsto coordinates,- shuttles data into the ancilla coord band
[npos, 2*npos),- rewrites the cloned op’s coords, and
- moves atoms back after the gate.
You still pay for two clones (
pbComb.op->clone()and thenopCopy->clone()intomappedQc). IfmappedQc.emplace_backcan take ownership of astd::unique_ptr<qc::Operation>, you could move the first clone directly to avoid the second allocation/copy.Not critical, but an easy micro-optimization on a hot path.
include/hybridmap/NeutralAtomArchitecture.hpp (2)
22-35: Potentially unused includes remain.Past review flagged
<array>,<cmath>,<iterator>, and<utility>as unused. These should be verified and removed if not needed to reduce compilation overhead.#!/bin/bash # Check usage of types from potentially unused headers echo "=== Checking std::array usage ===" rg -n "std::array" include/hybridmap/NeutralAtomArchitecture.hpp echo "=== Checking cmath function usage ===" rg -n "std::(sqrt|pow|abs|floor|ceil|round|sin|cos|exp|log|hypot)" include/hybridmap/NeutralAtomArchitecture.hpp echo "=== Checking iterator usage ===" rg -n "std::(iterator|advance|distance|next|prev)" include/hybridmap/NeutralAtomArchitecture.hpp echo "=== Checking utility usage ===" rg -n "std::(pair|make_pair|move|forward|swap|exchange)" include/hybridmap/NeutralAtomArchitecture.hpp
573-576:getVectorShuttlingTimestill usesOpType::Moveinstead ofOpType::AodMove.This issue was flagged in a past review. The JSON configurations define both
"move"and"aod_move"with potentially different values (e.g.,rubidium_hybrid.jsonhas"move": 0.55vs"aod_move": 0.3). UsingOpType::Movehere while other shuttling calculations useOpType::AodMovecauses inconsistent timing results.[[nodiscard]] qc::fp getVectorShuttlingTime(const MoveVector& v) const { return v.getLength() * getInterQubitDistance() / - getShuttlingTime(qc::OpType::Move); + getShuttlingTime(qc::OpType::AodMove); }src/hybridmap/MoveToAodConverter.cpp (1)
321-325: Unsigned subtraction fordeltacan still underflow.This issue was flagged in a past review. The code computes:
const auto start = static_cast<std::uint32_t>(...); const auto end = static_cast<std::uint32_t>(...); const auto delta = static_cast<qc::fp>(end - start);When
end < start, the unsigned subtraction wraps to a large positive value before the cast toqc::fp. This produces incorrect results in the merge-type comparisons at lines 363-364.Apply this diff to use signed arithmetic:
const auto start = static_cast<std::uint32_t>(dim == Dimension::X ? origin.x : origin.y); const auto end = static_cast<std::uint32_t>(dim == Dimension::X ? final.x : final.y); - const auto delta = static_cast<qc::fp>(end - start); + const auto delta = static_cast<qc::fp>( + static_cast<std::int32_t>(end) - static_cast<std::int32_t>(start));include/hybridmap/HybridNeutralAtomMapper.hpp (1)
612-615: Consider adding I/O error handling.The
saveMappedQcQasmmethod doesn't verify that the file opened successfully. While this was flagged in past review, it appears not fully addressed.[[maybe_unused]] void saveMappedQcQasm(const std::string& filename) const { std::ofstream ofs(filename); + if (!ofs) { + throw std::runtime_error("Failed to open file: " + filename); + } mappedQc.dumpOpenQASM(ofs, false); }include/hybridmap/MoveToAodConverter.hpp (1)
356-363: Loop index type mismatch with container size.The loop uses
std::uint32_t ibut compares againstflyingAncillas.getInitHwPos().size()which returnsstd::size_t. On 64-bit systems, this is a narrowing comparison. Additionally,i + 1used inAncillaAtomconstruction may need explicit casting for strict compilers.- for (std::uint32_t i = 0; i < flyingAncillas.getInitHwPos().size(); ++i) { + for (std::size_t i = 0; i < flyingAncillas.getInitHwPos().size(); ++i) { const auto coord = flyingAncillas.getInitHwPos().at(i) + (2 * arch.getNpositions()); const auto col = coord % arch.getNcolumns(); const auto row = coord / arch.getNcolumns(); - const AncillaAtom ancillaAtom({col, row}, {i + 1, i + 1}); + const AncillaAtom ancillaAtom( + {static_cast<std::uint32_t>(col), static_cast<std::uint32_t>(row)}, + {static_cast<std::uint32_t>(i + 1), static_cast<std::uint32_t>(i + 1)}); ancillas.emplace_back(ancillaAtom); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (26)
CHANGELOG.md(3 hunks)UPGRADING.md(1 hunks)bindings/hybrid_mapper/hybrid_mapper.cpp(4 hunks)docs/na_hybrid.md(1 hunks)include/hybridmap/HardwareQubits.hpp(6 hunks)include/hybridmap/HybridAnimation.hpp(1 hunks)include/hybridmap/HybridNeutralAtomMapper.hpp(3 hunks)include/hybridmap/HybridSynthesisMapper.hpp(1 hunks)include/hybridmap/Mapping.hpp(3 hunks)include/hybridmap/MoveToAodConverter.hpp(5 hunks)include/hybridmap/NeutralAtomArchitecture.hpp(4 hunks)include/hybridmap/NeutralAtomDefinitions.hpp(2 hunks)include/hybridmap/NeutralAtomLayer.hpp(2 hunks)include/hybridmap/NeutralAtomScheduler.hpp(3 hunks)include/hybridmap/NeutralAtomUtils.hpp(5 hunks)python/mqt/qmap/hybrid_mapper.pyi(3 hunks)src/hybridmap/HardwareQubits.cpp(7 hunks)src/hybridmap/HybridNeutralAtomMapper.cpp(30 hunks)src/hybridmap/HybridSynthesisMapper.cpp(1 hunks)src/hybridmap/Mapping.cpp(1 hunks)src/hybridmap/MoveToAodConverter.cpp(15 hunks)src/hybridmap/NeutralAtomArchitecture.cpp(8 hunks)src/hybridmap/NeutralAtomLayer.cpp(4 hunks)src/hybridmap/NeutralAtomScheduler.cpp(4 hunks)test/hybridmap/test_hybridmap.cpp(4 hunks)test/python/hybrid_mapper/test_hybrid_synthesis.py(1 hunks)
🧰 Additional context used
🧠 Learnings (17)
📓 Common learnings
Learnt from: lsschmid
Repo: munich-quantum-toolkit/qmap PR: 832
File: src/hybridmap/HybridNeutralAtomMapper.cpp:1679-1715
Timestamp: 2025-11-20T18:19:31.857Z
Learning: In src/hybridmap/HybridNeutralAtomMapper.cpp, the compareSwapAndBridge and compareShuttlingAndFlyingAncilla methods intentionally use division by zero to produce infinity when distance reductions are zero or negative. This infinity value marks that option as highly unfavorable in cost comparisons, which is the intended behavior rather than a bug.
📚 Learning: 2025-10-14T14:37:38.047Z
Learnt from: burgholzer
Repo: munich-quantum-toolkit/yaqs PR: 212
File: CHANGELOG.md:12-15
Timestamp: 2025-10-14T14:37:38.047Z
Learning: In the munich-quantum-toolkit/yaqs project, changelog entries follow the template: "- $TITLE ([#$NUMBER]($URL)) ([**AUTHOR**](https://github.com/$AUTHOR))". Issue references should not be included in changelog entries; the PR number is sufficient for traceability.
Applied to files:
CHANGELOG.md
📚 Learning: 2025-12-01T11:00:40.342Z
Learnt from: flowerthrower
Repo: munich-quantum-toolkit/core-plugins-catalyst PR: 1
File: CHANGELOG.md:18-18
Timestamp: 2025-12-01T11:00:40.342Z
Learning: In the munich-quantum-toolkit/core-plugins-catalyst repository, the CHANGELOG.md intentionally references the parent MQT Core repository's release notes (https://github.com/munich-quantum-toolkit/core/releases) because the plugin repository is based on work performed in the parent repository.
Applied to files:
CHANGELOG.md
📚 Learning: 2025-11-20T18:19:31.857Z
Learnt from: lsschmid
Repo: munich-quantum-toolkit/qmap PR: 832
File: src/hybridmap/HybridNeutralAtomMapper.cpp:1679-1715
Timestamp: 2025-11-20T18:19:31.857Z
Learning: In src/hybridmap/HybridNeutralAtomMapper.cpp, the compareSwapAndBridge and compareShuttlingAndFlyingAncilla methods intentionally use division by zero to produce infinity when distance reductions are zero or negative. This infinity value marks that option as highly unfavorable in cost comparisons, which is the intended behavior rather than a bug.
Applied to files:
UPGRADING.mdsrc/hybridmap/Mapping.cppinclude/hybridmap/HybridSynthesisMapper.hppsrc/hybridmap/NeutralAtomArchitecture.cppsrc/hybridmap/NeutralAtomLayer.cppinclude/hybridmap/HybridAnimation.hppinclude/hybridmap/Mapping.hppbindings/hybrid_mapper/hybrid_mapper.cppsrc/hybridmap/HybridSynthesisMapper.cppinclude/hybridmap/HybridNeutralAtomMapper.hppsrc/hybridmap/MoveToAodConverter.cppsrc/hybridmap/HardwareQubits.cppinclude/hybridmap/HardwareQubits.hppsrc/hybridmap/HybridNeutralAtomMapper.cpptest/hybridmap/test_hybridmap.cppinclude/hybridmap/NeutralAtomUtils.hppsrc/hybridmap/NeutralAtomScheduler.cppinclude/hybridmap/NeutralAtomArchitecture.hppinclude/hybridmap/MoveToAodConverter.hppinclude/hybridmap/NeutralAtomDefinitions.hpp
📚 Learning: 2025-11-20T16:57:24.305Z
Learnt from: lsschmid
Repo: munich-quantum-toolkit/qmap PR: 832
File: test/hybridmap/test_hybridmap.cpp:200-287
Timestamp: 2025-11-20T16:57:24.305Z
Learning: In test/hybridmap/test_hybridmap.cpp, the LongShuttling test is designed as a stress test to catch rare edge cases that only occur after extended shuttling sequences, which is why it maps a long circuit without explicit assertions beyond ensuring no exceptions are thrown.
Applied to files:
docs/na_hybrid.mdtest/python/hybrid_mapper/test_hybrid_synthesis.pysrc/hybridmap/Mapping.cppsrc/hybridmap/HybridSynthesisMapper.cppsrc/hybridmap/HybridNeutralAtomMapper.cpptest/hybridmap/test_hybridmap.cppinclude/hybridmap/NeutralAtomUtils.hpp
📚 Learning: 2025-11-27T08:52:06.428Z
Learnt from: lsschmid
Repo: munich-quantum-toolkit/qmap PR: 832
File: src/hybridmap/HybridNeutralAtomMapper.cpp:1379-1383
Timestamp: 2025-11-27T08:52:06.428Z
Learning: In src/hybridmap/HybridNeutralAtomMapper.cpp, the getBestMovePos function throws std::runtime_error when no move position is found (finalBestPos.coords is empty). This behavior is acceptable for current use cases since tight architectures are not a concern. A future improvement would be to fall back to SWAP-based strategies instead of throwing.
Applied to files:
include/hybridmap/NeutralAtomScheduler.hppsrc/hybridmap/Mapping.cppinclude/hybridmap/HybridSynthesisMapper.hppsrc/hybridmap/NeutralAtomArchitecture.cppsrc/hybridmap/NeutralAtomLayer.cppinclude/hybridmap/NeutralAtomLayer.hppinclude/hybridmap/HybridAnimation.hppinclude/hybridmap/Mapping.hppbindings/hybrid_mapper/hybrid_mapper.cppsrc/hybridmap/HybridSynthesisMapper.cppinclude/hybridmap/HybridNeutralAtomMapper.hppsrc/hybridmap/MoveToAodConverter.cppsrc/hybridmap/HardwareQubits.cppinclude/hybridmap/HardwareQubits.hppsrc/hybridmap/HybridNeutralAtomMapper.cpptest/hybridmap/test_hybridmap.cppinclude/hybridmap/NeutralAtomUtils.hppsrc/hybridmap/NeutralAtomScheduler.cppinclude/hybridmap/NeutralAtomArchitecture.hppinclude/hybridmap/MoveToAodConverter.hppinclude/hybridmap/NeutralAtomDefinitions.hpp
📚 Learning: 2025-10-07T15:30:42.946Z
Learnt from: MatthiasReumann
Repo: munich-quantum-toolkit/core PR: 1237
File: mlir/include/mlir/Dialect/MQTOpt/Transforms/Transpilation/Layout.h:219-231
Timestamp: 2025-10-07T15:30:42.946Z
Learning: In the Layout class for MLIR quantum routing (mlir/include/mlir/Dialect/MQTOpt/Transforms/Transpilation/Layout.h), the swap method intentionally does NOT swap the hw fields in QubitInfo. This is correct because SSA values represent quantum states at fixed hardware locations, and only their program index associations change during a SWAP gate. The hw field indicates where an SSA value physically resides and remains constant.
Applied to files:
src/hybridmap/Mapping.cppsrc/hybridmap/NeutralAtomArchitecture.cppinclude/hybridmap/Mapping.hppsrc/hybridmap/HybridSynthesisMapper.cppinclude/hybridmap/HybridNeutralAtomMapper.hppsrc/hybridmap/HardwareQubits.cppinclude/hybridmap/HardwareQubits.hppsrc/hybridmap/HybridNeutralAtomMapper.cppinclude/hybridmap/NeutralAtomDefinitions.hpp
📚 Learning: 2025-12-04T06:59:40.314Z
Learnt from: MatthiasReumann
Repo: munich-quantum-toolkit/core PR: 1301
File: mlir/lib/Dialect/MQTOpt/Transforms/Transpilation/LayeredUnit.cpp:84-85
Timestamp: 2025-12-04T06:59:40.314Z
Learning: In the MQTOpt MLIR routing passes (NaiveRoutingPassSC, AStarRoutingPassSC), the input IR is guaranteed to contain only 1-qubit and 2-qubit gates. All 3+-qubit gates must be decomposed before routing; otherwise the input IR is invalid. This invariant allows skipTwoQubitBlock in LayeredUnit.cpp to safely assert wires.size() == 2.
Applied to files:
src/hybridmap/Mapping.cppsrc/hybridmap/NeutralAtomArchitecture.cppsrc/hybridmap/NeutralAtomLayer.cppinclude/hybridmap/NeutralAtomLayer.hppinclude/hybridmap/Mapping.hppsrc/hybridmap/HardwareQubits.cppsrc/hybridmap/HybridNeutralAtomMapper.cpptest/hybridmap/test_hybridmap.cpp
📚 Learning: 2025-10-09T04:30:29.071Z
Learnt from: MatthiasReumann
Repo: munich-quantum-toolkit/core PR: 1237
File: mlir/include/mlir/Dialect/MQTOpt/Transforms/Transpilation/Scheduler.h:72-140
Timestamp: 2025-10-09T04:30:29.071Z
Learning: In the CrawlScheduler (mlir/include/mlir/Dialect/MQTOpt/Transforms/Transpilation/Scheduler.h), the persistent `layoutCopy` is updated across all lookahead layers via `remapQubitValue` calls. After each two-qubit gate is scheduled in a layer, its input qubits are remapped to output qubits, ensuring the next layer's crawl starts from those outputs. This progressive remapping through SSA values prevents the same operation from being scheduled in multiple lookahead layers.
Applied to files:
include/hybridmap/HybridSynthesisMapper.hppsrc/hybridmap/NeutralAtomLayer.cppinclude/hybridmap/HybridNeutralAtomMapper.hppsrc/hybridmap/HybridNeutralAtomMapper.cppsrc/hybridmap/NeutralAtomScheduler.cpp
📚 Learning: 2025-11-01T15:57:31.153Z
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1283
File: src/qir/runtime/QIR.cpp:196-201
Timestamp: 2025-11-01T15:57:31.153Z
Learning: In the QIR runtime (src/qir/runtime/QIR.cpp), the PRX gate (__quantum__qis__prx__body) is an alias for the R gate (Phased X-Rotation) and should call runtime.apply<qc::R>(theta, phi, qubit), not runtime.apply<qc::RX>() which is a single-parameter rotation gate.
Applied to files:
src/hybridmap/NeutralAtomArchitecture.cpptest/hybridmap/test_hybridmap.cpp
📚 Learning: 2025-10-09T13:20:11.483Z
Learnt from: DRovara
Repo: munich-quantum-toolkit/core PR: 1108
File: mlir/test/Dialect/MQTOpt/Transforms/lift-measurements.mlir:269-288
Timestamp: 2025-10-09T13:20:11.483Z
Learning: In the MQT MLIR dialect, the `rz` gate should not be included in the `DIAGONAL_GATES` set for the `ReplaceBasisStateControlsWithIfPattern` because its operator matrix does not have the required shape | 1 0 | / | 0 x | for the targets-as-controls optimization. It is only included in `LiftMeasurementsAboveGatesPatterns` where the matrix structure requirement differs.
Applied to files:
src/hybridmap/NeutralAtomArchitecture.cppsrc/hybridmap/HybridNeutralAtomMapper.cpp
📚 Learning: 2025-10-09T13:13:51.224Z
Learnt from: DRovara
Repo: munich-quantum-toolkit/core PR: 1108
File: mlir/lib/Dialect/MQTOpt/Transforms/ReplaceBasisStateControlsWithIfPattern.cpp:171-180
Timestamp: 2025-10-09T13:13:51.224Z
Learning: In MQT Core MLIR, UnitaryInterface operations guarantee 1-1 correspondence between input and output qubits in the same order. When cloning or modifying unitary operations (e.g., removing controls), this correspondence is maintained by construction, so yielding getAllInQubits() in else-branches matches the result types from the operation's outputs.
Applied to files:
src/hybridmap/NeutralAtomArchitecture.cppsrc/hybridmap/NeutralAtomLayer.cppinclude/hybridmap/Mapping.hppinclude/hybridmap/HardwareQubits.hppsrc/hybridmap/HybridNeutralAtomMapper.cpp
📚 Learning: 2025-11-05T09:23:46.540Z
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1287
File: src/qdmi/dd/Device.cpp:492-521
Timestamp: 2025-11-05T09:23:46.540Z
Learning: In the munich-quantum-toolkit/core repository, the `qasm3::Importer::imports()` function is backwards compatible with OpenQASM 2 programs. Therefore, it can be used to parse both QASM2 and QASM3 program formats without requiring separate importers for each version.
Applied to files:
bindings/hybrid_mapper/hybrid_mapper.cpppython/mqt/qmap/hybrid_mapper.pyi
📚 Learning: 2025-11-24T10:19:41.147Z
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1326
File: python/mqt/core/__init__.py:22-22
Timestamp: 2025-11-24T10:19:41.147Z
Learning: In the munich-quantum-toolkit/core repository, Ruff is configured with 'ALL' rules enabled by default, and only specific rules are selectively disabled. When reviewing changes that enable previously-disabled rules (like PLC0415), noqa directives for those rules become necessary and should be retained.
Applied to files:
include/hybridmap/HybridNeutralAtomMapper.hpp
📚 Learning: 2025-11-05T07:42:45.507Z
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1287
File: test/qdmi/dd/error_handling_test.cpp:118-194
Timestamp: 2025-11-05T07:42:45.507Z
Learning: In the munich-quantum-toolkit/core QDMI device API, session parameters can only be set before calling `device_session_init()`. Once a session is initialized, any attempt to set a parameter returns `QDMI_ERROR_BADSTATE`. Since `SessionGuard` (in test/qdmi/dd/helpers/test_utils.hpp) automatically initializes the session in its constructor, tests that need to verify session parameter setting behavior before initialization must allocate a separate uninitialized session rather than reusing the `SessionGuard`'s session.
Applied to files:
include/hybridmap/HybridNeutralAtomMapper.hpptest/hybridmap/test_hybridmap.cpp
📚 Learning: 2025-12-02T07:37:46.860Z
Learnt from: MatthiasReumann
Repo: munich-quantum-toolkit/core PR: 1301
File: mlir/lib/Dialect/MQTOpt/Transforms/Transpilation/sc/NaiveRoutingPass.cpp:144-151
Timestamp: 2025-12-02T07:37:46.860Z
Learning: In MLIR transformation code (mlir/lib/Dialect/MQTOpt/Transforms/Transpilation/sc/NaiveRoutingPass.cpp and similar), when inserting operations before a target operation, prefer `rewriter.setInsertionPoint(op)` over `rewriter.setInsertionPointAfter(op->getPrevNode())`. The former is cleaner, avoids null pointer edge cases (when op is first in block), and is semantically clearer.
Applied to files:
src/hybridmap/MoveToAodConverter.cppsrc/hybridmap/HybridNeutralAtomMapper.cppinclude/hybridmap/NeutralAtomUtils.hpp
📚 Learning: 2025-10-07T15:58:19.247Z
Learnt from: MatthiasReumann
Repo: munich-quantum-toolkit/core PR: 1237
File: mlir/include/mlir/Dialect/MQTOpt/Transforms/Transpilation/Planner.h:202-214
Timestamp: 2025-10-07T15:58:19.247Z
Learning: In the QMAP routing algorithm (mlir/include/mlir/Dialect/MQTOpt/Transforms/Transpilation/Planner.h), the heuristic function h() intentionally uses a non-admissible heuristic that sums per-gate shortest-path distances. This is a deliberate design choice from the QMAP paper, where admissibility is dropped because locally optimal solutions can negatively affect the overall routing quality. This approach prioritizes global solution quality over guaranteed local optimality.
Applied to files:
src/hybridmap/HybridNeutralAtomMapper.cpp
🧬 Code graph analysis (8)
include/hybridmap/NeutralAtomScheduler.hpp (1)
include/hybridmap/HybridAnimation.hpp (1)
initHwPos(53-54)
test/python/hybrid_mapper/test_hybrid_synthesis.py (1)
python/mqt/qmap/hybrid_mapper.pyi (17)
HybridSynthesisMapper(101-121)NeutralAtomHybridArchitecture(50-79)init_mapping(114-114)evaluate_synthesis_steps(107-109)append_with_mapping(103-103)append_without_mapping(104-104)get_mapped_qc_qasm(85-85)get_mapped_qc_qasm(111-111)save_mapped_qc_qasm(115-115)convert_to_aod(106-106)get_mapped_qc_aod_qasm(86-86)get_mapped_qc_aod_qasm(112-112)save_mapped_qc_aod_qasm(91-91)save_mapped_qc_aod_qasm(116-116)get_synthesized_qc_qasm(113-113)save_synthesized_qc_qasm(117-117)get_circuit_adjacency_matrix(110-110)
src/hybridmap/NeutralAtomArchitecture.cpp (1)
include/hybridmap/NeutralAtomArchitecture.hpp (8)
Properties(69-69)Properties(82-92)NeutralAtomArchitecture(216-216)interactionRadius(197-197)op(329-329)op(335-335)op(342-342)v(573-573)
src/hybridmap/NeutralAtomLayer.cpp (1)
include/hybridmap/NeutralAtomLayer.hpp (5)
qubitsToUpdate(57-57)qubitsToUpdate(67-67)qubitsToUpdate(76-76)commutesWithAtQubit(138-139)commuteAtQubit(150-151)
include/hybridmap/HybridNeutralAtomMapper.hpp (3)
include/hybridmap/HardwareQubits.hpp (5)
op(215-220)op(215-215)HardwareQubits(88-88)HardwareQubits(100-143)HardwareQubits(100-103)test/hybridmap/test_hybridmap.cpp (5)
mapper(94-94)mapper(227-227)mapper(263-263)mapper(272-272)mapper(285-285)include/hybridmap/NeutralAtomScheduler.hpp (1)
qc(136-140)
src/hybridmap/HybridNeutralAtomMapper.cpp (4)
include/hybridmap/HardwareQubits.hpp (13)
op(215-220)op(215-215)q(300-300)q(309-309)hwQubit(179-179)hwQubit(187-209)hwQubit(187-187)qubit(70-70)qubit(228-228)qubits(332-332)qubits(363-363)coord(343-344)coord(353-354)include/hybridmap/Mapping.hpp (10)
op(199-204)op(199-199)qubit(113-115)qubit(113-113)qubit(124-124)qubit(170-170)qubit(187-187)qubits(136-136)qubits(152-152)swap(213-213)include/sc/Architecture.hpp (5)
control(427-428)qubit(88-93)qubit(88-90)qubit(95-96)qubit(101-101)src/hybridmap/HardwareQubits.cpp (2)
move(137-180)move(137-137)
test/hybridmap/test_hybridmap.cpp (2)
include/hybridmap/NeutralAtomArchitecture.hpp (1)
NeutralAtomArchitecture(216-216)include/hybridmap/HybridNeutralAtomMapper.hpp (13)
qc(549-556)qc(549-550)qc(558-563)qc(558-559)qc(571-571)qc(678-678)NeutralAtomMapper(447-447)NeutralAtomMapper(448-469)NeutralAtomMapper(448-449)NeutralAtomMapper(470-472)NeutralAtomMapper(470-471)p(480-493)p(480-480)
include/hybridmap/MoveToAodConverter.hpp (2)
include/hybridmap/NeutralAtomArchitecture.hpp (17)
c(418-418)nodiscard(97-99)nodiscard(103-103)nodiscard(107-107)nodiscard(111-111)nodiscard(115-117)nodiscard(121-123)nodiscard(127-129)nodiscard(133-135)nodiscard(139-139)nodiscard(161-166)nodiscard(231-231)nodiscard(236-238)nodiscard(243-245)nodiscard(257-259)nodiscard(264-264)v(573-573)src/hybridmap/HardwareQubits.cpp (2)
move(137-180)move(137-137)
🪛 LanguageTool
CHANGELOG.md
[grammar] ~18-~18: Ensure spelling is correct
Context: ...m Compiler ([#847]) ([@ystade]) - ✨ Added extension to the hybrid routing mapper ...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~19-~19: Ensure spelling is correct
Context: ...ancillas ([#832]) ([@lsschmid]) - ✨ Added hybrid synthesis routing for iterative ...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
UPGRADING.md
[grammar] ~15-~15: Ensure spelling is correct
Context: ...uting. Additionally, we also introduce and extension to the Hybrid Neutral Atom Ma...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
docs/na_hybrid.md
[style] ~20-~20: The abbreviation “ASAP” (= as soon as possible) can sound harsh. Consider an alternative.
Context: ... capability-specific cost models and an ASAP scheduler that respects hardware constr...
(ASAP)
[grammar] ~50-~50: Please add a punctuation mark at the end of paragraph.
Context: ...s repository ships several ready-to-use examples ```{code-cell} ipython3 from pathlib ...
(PUNCTUATION_PARAGRAPH_END)
[style] ~227-~227: Consider using the typographical ellipsis character here instead.
Context: ...ile("path/to/circuit.qasm")and thenmapper.schedule(...)` as shown above. - Architectures: the...
(ELLIPSIS)
[grammar] ~234-~234: Please add a punctuation mark at the end of paragraph.
Context: ...irrors the unit test flow and shows how to - evaluate multiple candidate fragment...
(PUNCTUATION_PARAGRAPH_END)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: 🐍 Test (macos-14) / 🐍 macos-14
- GitHub Check: 🐍 Test (macos-15-intel) / 🐍 macos-15-intel
- GitHub Check: 🐍 Test (windows-2022) / 🐍 windows-2022
🔇 Additional comments (70)
CHANGELOG.md (1)
18-19: LGTM!The changelog entries follow the project's established format with proper PR link and contributor attribution. The entries clearly describe the new hybrid mapping extension features (Bridge gates, Passby moves, Flying ancillas) and hybrid synthesis routing.
include/hybridmap/HardwareQubits.hpp (1)
187-209: LGTM!The
removeHwQubitimplementation correctly captures the current coordinate before erasing mappings, properly updates bothoccupiedCoordinatesandfreeCoordinates, invalidates swap distances, and cleans up nearby qubit references. This addresses the issues flagged in previous reviews.python/mqt/qmap/hybrid_mapper.pyi (2)
81-98: LGTM!The
HybridNAMappertype stubs are well-defined with clear method signatures. The*_qasmsuffix on method names makes the return type (QASM strings) explicit, which addresses the earlier review feedback about consistency.
100-121: LGTM!The
HybridSynthesisMappertype stubs are consistent withHybridNAMapperand provide a complete API surface for synthesis-aware mapping workflows. Method naming conventions (e.g.,get_*_qasm,save_*_qasm) clearly indicate return types and purposes.UPGRADING.md (1)
17-21: LGTM!The upgrade documentation clearly explains backward compatibility by specifying default parameter values (
usePassBy=False,numFlyingAncillas=0,maxBridgeDistance=0) that recover previous behavior. The.navizoutput capability is also documented.include/hybridmap/NeutralAtomScheduler.hpp (4)
43-44: LGTM!The
SchedulerResultsstruct is properly extended withnAodActivateandnAodMovecounters. The constructor initializes all new fields, andtoMap()correctly exports them with appropriate type casting.Also applies to: 58-64, 91-92
111-117: LGTM!Deleting the default constructor and requiring explicit architecture initialization addresses the earlier review concern about potential null pointer dereferences in
schedule(). The explicit constructor correctly stores the architecture pointer.
157-163: Docstring mentions.nastylebut implementation doesn't write it.The documentation at line 159 states the method writes
.naviz/.namachineartifacts, which matches the implementation. However, a past review noted that.nastylewas mentioned elsewhere. Please verify the docstring accurately reflects the actual outputs.
164-183: LGTM!The
saveAnimationFilesimplementation includes appropriate guards with a warning when animation data is missing, and correctly derives output filenames from the input path. The early return when either animation string is empty prevents writing incomplete artifacts.docs/na_hybrid.md (1)
1-285: Documentation looks comprehensive and well-structured.The documentation covers the HyRoNA workflow end-to-end with clear examples for architecture loading, mapping, scheduling, animation export, parameter tuning, and the Hybrid Synthesis Mapper. Previous review feedback has been addressed (corrected
ipython3directive, clarifiedshuttling_weightsemantics, limited QASM snippets, fixed AOD coordinate explanation, and aligned animation output description).src/hybridmap/Mapping.cpp (2)
27-42: LGTM:applySwapcorrectly handles all mapping cases.The refactored function properly handles the four cases:
- Both qubits mapped → swap their circuit qubit assignments
- Only q1 mapped → transfer mapping to q2
- Only q2 mapped → transfer mapping to q1
- Neither mapped → throw (invalid operation)
The const reference parameter and local const bindings improve clarity.
44-189: Graph matching algorithm is well-structured.The implementation correctly:
- Builds hardware and circuit adjacency graphs with weighted edges
- Identifies center qubits by connectivity
- Prioritizes circuit qubits by degree and total edge weight
- Uses weighted distance minimization for placement decisions
- Handles neighbor mapping to fill adjacent positions
The algorithm provides a reasonable greedy heuristic for initial placement.
test/python/hybrid_mapper/test_hybrid_synthesis.py (1)
1-131: Test coverage is solid for the HybridSynthesisMapper Python bindings.The tests properly cover:
- Synthesis step evaluation with multiple architectures
- I/O operations (QASM export, AOD conversion, file saving) using
tmp_path- Adjacency matrix retrieval and symmetry
- Object lifetime with helper functions
Previous review feedback has been addressed (keyword argument for
also_map,tmp_pathfixture usage).include/hybridmap/HybridSynthesisMapper.hpp (1)
32-150: Header is well-structured with clear documentation.The API is well-documented with:
- Clear class-level documentation explaining the bridging role
- Per-method documentation with parameter and return descriptions
- Appropriate use of
[[nodiscard]]and[[maybe_unused]]attributes- Logical grouping of constructors, lifecycle methods, accessors, and operations
The
completeRemapfix (using a copy) addresses the previous mutation concern.src/hybridmap/HybridSynthesisMapper.cpp (3)
31-68:evaluateSynthesisStepsis well-implemented.The function properly:
- Guards against empty input (returns early)
- Initializes on first use
- Computes fidelity before emplacing (avoiding evaluation order issues)
- Uses proper logging via spdlog
- Guards
alsoMapwith!candidates.empty()Previous review feedback has been addressed.
70-80:evaluateSynthesisStepcorrectly preserves input.The function now makes a copy of
qcbefore mapping, ensuring the original circuit is not mutated. This addresses the previous const-correctness concern.
103-129: Adjacency matrix implementation is correct.The function:
- Validates initialization state
- Properly sets both
(i,j)and(j,i)for symmetry- Explicitly sets diagonal to 0
- Uses swap distance of 0 to determine adjacency
src/hybridmap/NeutralAtomLayer.cpp (4)
18-21: LGTM! Modern include update.Replacing
<iterator>with<cstddef>is appropriate sincestd::size_tis the primary utility needed here.
42-69: Front-layer vs lookahead logic is well-structured.The branching between front-layer mode (commutation-based candidate collection) and lookahead mode (bounded multi-qubit gate collection) is clear. The commutation checks at lines 46-47 properly gate candidate addition.
One observation: in lookahead mode (lines 57-67), single-qubit gates are collected without limit between multi-qubit gates. This appears intentional for heuristic evaluation but may accumulate many candidates for circuits with long single-qubit sequences.
72-111: Candidate promotion logic is correct; consider iterator validity note.The logic correctly:
- Checks if an operation is a candidate on all its qubits (lines 80-90)
- Promotes to gates and newGates (lines 92-93)
- Removes from other qubits' candidate lists (lines 95-101)
- Defers removal from the current qubit's list to avoid iterator invalidation (lines 106-110)
The use of
std::ranges::findfollowed byeraseat lines 99-100 assumes the element exists (since we just verified candidacy). This is safe given the preceding check.
114-125: Gate removal with ranges is clean.The refactored
removeGatesAndUpdateproperly:
- Uses
std::ranges::findfor safe lookup- Only erases if found (lines 118-119)
- Collects affected qubits for subsequent update
This is a good modernization from the previous iterator-based approach.
include/hybridmap/NeutralAtomDefinitions.hpp (4)
13-21: LGTM! Required includes are present.The addition of
<tuple>supports thestd::tieusage inoperator<, and the other includes properly support the new types.
29-36: Good documentation for CoordIndex and AdjacencyMatrix.The doxygen comments clearly explain the purpose of
CoordIndexas a physical coordinate slot index. TheAdjacencyMatrixalias is appropriate for the symmetric matrix representation.
79-110: AtomMove struct is well-designed with proper ordering.The struct provides:
- Clear field documentation (c1/c2 for coordinates, load1/load2 for actions)
- Proper equality operators
- Strict weak ordering via
std::tiefor use in ordered containersThis addresses the previous review concern about missing
operator<.
112-119: GateList and GateLists type aliases are appropriate.These provide clear semantic names for operation pointer vectors and their collections, improving code readability throughout the codebase.
include/hybridmap/HybridAnimation.hpp (5)
24-33: Lifetime contract is now properly documented.The
@noteat lines 31-32 clearly states that "The architecture reference passed to the constructor must remain valid for the lifetime of this object." This addresses the previous review concern about undocumented lifetime requirements.
34-40: Raw pointer storage is acceptable with documented contract.The
archmember stores a non-owning pointer initialized from a reference in the constructor. Given the explicit lifetime documentation, this design is appropriate for performance-sensitive code where the architecture is expected to outlive all animation helpers.
42-54: initPositions documentation is clear.The protected helper is well-documented, explaining the coordinate initialization for both hardware qubits and flying ancillas, including the offset calculations based on AOD intermediate levels.
64-69: Constructor properly initializes architecture pointer.The constructor takes a reference and stores its address, which is safe given the documented lifetime contract. The immediate call to
initPositionsensures consistent state after construction.
71-95: opToNaViz documentation is comprehensive.The docstring now explicitly covers:
- AOD operations (Activate, Deactivate, Move)
- Multi-qubit gates (grouped cz)
- Single-qubit gates (rz visualization)
- Explicitly states "Other operations are currently not supported"
This addresses the previous concern about undocumented operation handling.
include/hybridmap/NeutralAtomLayer.hpp (4)
24-31: Class documentation clearly explains the dual-mode design.The updated docstring effectively communicates the two operational modes:
- Front layer: mutually commuting gates ready to execute
- Look-ahead layer: bounded depth of forthcoming multi-qubit gates
This provides good context for users of the class.
39-46: New state members support lookahead functionality.The additions are appropriate:
ends: tracks DAG column end iterators for bounds checkingnewGates: tracks gates added in the latest update (replacingmappedSingleQubitGates)lookaheadDepth: configurable multi-qubit gate depth for lookaheadisFrontLayer: mode flagThe rename from
mappedSingleQubitGatestonewGatesbetter reflects its actual purpose (all newly promoted gates, not just single-qubit).
105-112: Accessor methods are appropriately const-qualified.Both
getGates()andgetNewGates()are marked[[nodiscard]]andconst, correctly indicating they don't modify state and their return values should be used. Returning copies is reasonable for these small vectors.
128-151: Free function declarations are well-documented.The namespace-level commutation helpers have clear documentation explaining:
- Their purpose (checking commutation at a specific qubit)
- The rules applied (non-unitaries, identities, single-qubit ops, control/target patterns)
Making these free functions rather than class members is appropriate since they operate on general
GateListandqc::Operation*types.src/hybridmap/NeutralAtomArchitecture.cpp (7)
76-91: Robust fallback mechanism for required gate entries.The
ensureGateWithFallbacklambda elegantly handles missing gate entries by:
- Checking if the required gate exists
- Falling back to "none" if available
- Throwing with a clear error message if both are missing
This prevents silent failures and provides clear error messages during JSON loading.
107-132: SWAP and Bridge gate precomputation is safe and efficient.The implementation correctly:
- Computes SWAP timing from 3 CZ + 4 H gates (line 107-108)
- Computes SWAP fidelity from gate fidelities raised to appropriate powers (lines 109-111)
- Uses
std::minacross all bridge circuit vectors to prevent out-of-bounds access (lines 117-120)- Iterates only up to the safe maximum index
This addresses the previous review concern about bridge circuit index safety.
171-231: computeSwapDistances is well-implemented.The algorithm:
- Computes diagonal distances within interaction radius
- Sorts by distance for greedy diagonal reduction
- Uses a greedy approach to minimize swap count
Note at lines 225-227: if no diagonal moves were found,
swapDistancedefaults to 1. The stored value isswapDistance - 1, meaning adjacent qubits within interaction radius get distance 0 (direct interaction). This appears intentional for the hybrid mapping model.
265-319: getAnimationMachine implementation is robust.The method:
- Validates
shuttlingSpeedFactor > 0to prevent division by zero (lines 267-271)- Generates complete NaViz machine configuration including movement, timing, distance, zones, and trap positions
The use of
getGateTime("x")for rz/ry timing (lines 286-287) is acceptable per the developer's confirmation that all architectures define either "x" or "none".
333-348: getOpTime correctly handles multi-qubit and phase gates.The implementation:
- Uses safe iteration
for (size_t i = 1; i < nQubits; ++i)avoiding unsigned underflow (lines 334-335)- Handles P/RZ gates with proper pi-normalized scaling using
std::fmod(lines 342-347)This addresses the previous critical concern about unsigned underflow when
getNqubits() == 0.
359-365: getOpFidelity uses same safe loop pattern.The loop at lines 360-361 mirrors the safe pattern from
getOpTime, preventing underflow for operations with zero qubits.
368-396: getBlockedCoordIndices correctly handles ancilla register mapping.The implementation:
- Maps ancilla qubits back to primary positions (lines 378-380)
- Computes blocked indices based on Euclidean distance and blocking factor
- Extends blocking to ancilla register offsets (lines 390-391)
The
whileloop at lines 378-380 handles multiple ancilla register levels by repeatedly subtractinggetNpositions()until the coordinate is in the primary range. This is correct for the multi-level ancilla model.src/hybridmap/HardwareQubits.cpp (1)
91-130: BFS-basedcomputeAllShortestPathsandgetClosestQubitbehavior look correctThe new BFS over
nearbyQubitscorrectly enumerates all simple shortest paths between two hardware qubits, andgetClosestQubitnow robustly throws when no non‑ignored qubit exists instead of silently returning 0. This fixes the earlier latent bug and gives callers a well‑defined “no candidate” signal while reusing the same distance metric infrastructure.include/hybridmap/Mapping.hpp (1)
81-107: NewMappingconstructor fixes prior safety and performance issuesTaking
qc::QuantumComputation&instead of by-value and validating bothqc.getNqubits() <= hwQubits.getNumQubits()andnQubits <= qc.getNqubits()removes the redundant circuit copy and closes the out‑of‑bounds hole noted earlier for mismatchednQubits. The Graph branch then safely indexesqubitIndices[i]only within the validated range.src/hybridmap/HybridNeutralAtomMapper.cpp (1)
607-636:computeCurrentCoordUsagessizing and indexing now look safeSizing
coordUsagesas3 * arch->getNpositions()and then indexing it with:
- coordinates derived via
hardwareQubits.getCoordIndex(mapping.getHwQubit(qubit))(in‑range [0, nPositions)),- coord indices from
mappedQc’sgetUsedQubits()(which live in the 3‑register space), and- coord indices from
lastBlockedQubitsrouted viagetCoordIndex(qubit)fixes the earlier mismatch where flying‑ancilla and ancilla coords could exceed the vector size. This eliminates the previous out‑of‑bounds risk.
test/hybridmap/test_hybridmap.cpp (3)
59-134: Mapper parametrization and identity-mapping test look solidThe new
NeutralAtomMapperTestParamsfixture wires all relevantMapperParameters(includinginitialCoordMappinganddynamicMappingWeight), runs mapping +convertToAod(), and then schedules the circuit, asserting sensible lower bounds on execution time and fidelities. This gives good coverage of the end‑to‑end hybrid mapping/scheduling path for multiple architectures and circuits.
136-216: Output test robustly exercises mapping, I/O, stats, and animation
NeutralAtomMapperTest.Outputnow:
- writes mapped and AOD QASM to temp files and checks they exist and are non‑empty,
- verifies stats and stats map are non‑negative,
- confirms
getInitHwPos()size matches “nqubits minus flying ancilla”,- schedules with animation enabled and asserts non‑empty animation plus valid
.namachine/.navizfiles.Combined, this is a strong integration test for hybrid mapping, AOD conversion, scheduler, and NaViz export.
218-290: Exception tests nicely document and guard key failure modesThe new
NeutralAtomMapperExceptionstests (insufficient qubits for circuit+ancillas, enforcing a single flying ancilla, no free coords for shuttling in ctor/setParameters, no multi‑qubit space, and the long‑shuttling stress case) clearly encode the expected failure behavior of the mapper API and will help catch regressions in edge conditions.include/hybridmap/NeutralAtomUtils.hpp (3)
30-102: Configuration enums and parsers are clear and fail-fastThe introduction of
InitialCoordinateMapping,InitialMapping, andMappingMethodplus the*_FromStringhelpers (with explicit, case‑sensitive accepted tokens and descriptivestd::invalid_argumentmessages) makes mapper configuration much more explicit and robust than raw strings or ints.
214-268: Ancilla combo and move-combination structs are safer and more expressive nowDefault‑initializing
FlyingAncillaComb::opandPassByComb::optonullptr, adding cost/op/bestPos toMoveComb, and providing helper ctors/append/size/empty all make the ancilla/pass‑by/move bookkeeping clearer while avoiding uninitialized pointer UB. This pairs well with the new higher‑level mapping heuristics that rely on these types.
400-468:BridgeCircuitsprecomputation matches bridge-gate usage assumptionsPrecomputing bridge circuits and their H/CZ counts and depths for lengths in
[3, maxLength)and storing them inbridgeCircuits/hs/czs/hDepth/czDepthaligns with the architecture’s bridge‑gate decomposition needs. As long asmaxLengthis chosen to cover the maximum supported bridge size (which is already constrained bymaxBridgeDistanceon the mapper side), this design keeps runtime bridge decomposition simple and fast.bindings/hybrid_mapper/hybrid_mapper.cpp (5)
52-94: LGTM! Clean parameter binding structure.The
MapperParametersbindings are well-organized with descriptive docstrings for each field. The default constructor anddef_readwritebindings provide a clean Python interface.
96-107: LGTM!MapperStatsprovides useful mapping statistics.Clear exposure of mapping statistics with appropriate docstrings for each counter.
170-176: Good fix:keep_aliveadded toset_parameters.The
py::keep_alive<1, 2>()directive on line 176 correctly keeps theMapperParametersobject alive for the mapper's lifetime, addressing the previously identified dangling pointer risk.
286-300: Adjacency matrix export is clean and pre-sized.Good implementation following past review suggestion to pre-size the matrix. The conversion from
SymmetricMatrixtostd::vector<std::vector<int>>is correct.
230-237:keep_alivedirective forparamsis redundant but not problematic.The
py::keep_alive<1, 3>()directive is unnecessary becauseNeutralAtomMapperstoresMapperParametersby value (member:MapperParameters parametersat line 109), not by reference or pointer. The constructor copies the parameter via member initialization (parameters(p)at line 450), so the temporary created from the default argument is safely copied before being destroyed. The binding is safe despite the default argument.Likely an incorrect or invalid review comment.
include/hybridmap/HybridNeutralAtomMapper.hpp (4)
109-109: Good:MapperParametersnow stored by value.This change from
const MapperParameters*toMapperParametersby value eliminates the dangling pointer risks identified in past reviews. The mapper now owns its configuration.
447-469: Constructor safety improvements look good.The refactored constructor:
- Uses signed arithmetic for position checks (lines 454-456)
- Validates
numFlyingAncillasconstraint (lines 461-463)- Stores parameters by value, eliminating lifetime issues
This addresses the critical issues identified in past reviews.
480-493:setParametersnow properly validates and copies.The method correctly:
- Copies parameters by value (line 481)
- Uses signed arithmetic for the shuttling check (lines 482-484)
- Validates
numFlyingAncillas > 1(lines 489-491)This mirrors the constructor validation and addresses the past review concern about missing the flying ancilla check.
500-510:copyStateFromnow includesflyingAncillas.Line 509 adds the missing
flyingAncillascopy, addressing the past review concern about incomplete state copying.src/hybridmap/MoveToAodConverter.cpp (4)
69-81:convertOpToMovehandles flying ancilla coordinate mapping.The logic correctly maps qubit indices that exceed
getNpositions()back to valid coordinates while tracking theload1/load2flags. The while loops handle multiple position wrapping.
82-116:initFlyingAncillasemits activation for ancilla positions.The method constructs AOD activation operations for flying ancillas with their offset coordinates. The coordinate calculation appears correct, using the architecture's intermediate levels.
537-549: Iterator UB fix applied correctly.The past review flagged
targetQubits.insert(pos - 1, ...)as UB whenpos == begin(). The fix now correctly inserts atpos(lines 546-547), avoiding the invalid iterator decrement.
437-476:processMovescleanly separates standard moves from flying ancilla moves.The method correctly handles activation merging and falls back to creating a new move group when merging is impossible. The separation of concerns is clear.
include/hybridmap/NeutralAtomArchitecture.hpp (4)
88-92: Good: Validation for AOD intermediate levels.The assertions at lines 89 and 91 prevent division-by-zero in
getOffsetDistance()by ensuringaodDist > 0andnAodIntermediateLevels >= 1. This addresses a past review concern.
184-185: Bridge circuit limit now documented.The comment now correctly states "distances (3-9)" matching the
BridgeCircuits(10)initialization, addressing past review feedback about the misleading "<=5" comment.
353-359: Logging now uses spdlog instead of std::cout.
SPDLOG_WARNreplaces directstd::coutoutput, which is appropriate for library code. The same pattern is applied togetGateAverageFidelity.
427-431: Bounds check added forgetBridgeCircuit.The
assert(length < bridgeCircuits.bridgeCircuits.size())prevents out-of-bounds access, addressing the past review concern.include/hybridmap/MoveToAodConverter.hpp (3)
51-66:AncillaAtom::XAndYnow uses standard types.The struct correctly uses
std::uint32_tinstead of the non-portableuinttype, and the<sys/types.h>include has been removed. This addresses past review feedback.
81-95:AodMoveconstructor properly initializes all fields.The removal of the default constructor and use of a parameterized constructor ensures all members (
init,load,offset,delta) are properly initialized, eliminating the uninitialized-member risk flagged in past review.
285-294:getFirstIdxnow has defensive assertion.The
assert(!moves.empty() || !movesFa.empty())at line 286 prevents UB from dereferencing.front()on empty containers, addressing the past review concern.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
UPGRADING.md (1)
15-15: Fix typo: "and extension" → "an extension".This issue was previously flagged and remains unfixed.
Apply this diff:
-Additionally, we also introduce and extension to the Hybrid Neutral Atom Mapper (HyRoNA), which unifies gate-based routing (SWAP/BRIDGE) with atom shuttling, pass-by, and an optional flying ancilla to find the most suitable routing. +Additionally, we also introduce an extension to the Hybrid Neutral Atom Mapper (HyRoNA), which unifies gate-based routing (SWAP/BRIDGE) with atom shuttling, pass-by, and an optional flying ancilla to find the most suitable routing.include/hybridmap/HybridNeutralAtomMapper.hpp (1)
612-615: Add file I/O error checking for consistency.While
saveMappedQcAodQasmincludes file open validation (lines 639-641), this method lacks similar error handling. For API consistency and robustness, add the same validation pattern.Apply this diff:
[[maybe_unused]] void saveMappedQcQasm(const std::string& filename) const { std::ofstream ofs(filename); + if (!ofs) { + throw std::runtime_error("Failed to open file: " + filename); + } mappedQc.dumpOpenQASM(ofs, false); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
UPGRADING.md(1 hunks)docs/na_hybrid.md(1 hunks)include/hybridmap/HybridNeutralAtomMapper.hpp(3 hunks)
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: lsschmid
Repo: munich-quantum-toolkit/qmap PR: 832
File: src/hybridmap/HybridNeutralAtomMapper.cpp:1679-1715
Timestamp: 2025-11-20T18:19:31.857Z
Learning: In src/hybridmap/HybridNeutralAtomMapper.cpp, the compareSwapAndBridge and compareShuttlingAndFlyingAncilla methods intentionally use division by zero to produce infinity when distance reductions are zero or negative. This infinity value marks that option as highly unfavorable in cost comparisons, which is the intended behavior rather than a bug.
Learnt from: lsschmid
Repo: munich-quantum-toolkit/qmap PR: 832
File: src/hybridmap/HybridNeutralAtomMapper.cpp:1379-1383
Timestamp: 2025-11-27T08:52:06.428Z
Learning: In src/hybridmap/HybridNeutralAtomMapper.cpp, the getBestMovePos function throws std::runtime_error when no move position is found (finalBestPos.coords is empty). This behavior is acceptable for current use cases since tight architectures are not a concern. A future improvement would be to fall back to SWAP-based strategies instead of throwing.
📚 Learning: 2025-11-20T18:19:31.857Z
Learnt from: lsschmid
Repo: munich-quantum-toolkit/qmap PR: 832
File: src/hybridmap/HybridNeutralAtomMapper.cpp:1679-1715
Timestamp: 2025-11-20T18:19:31.857Z
Learning: In src/hybridmap/HybridNeutralAtomMapper.cpp, the compareSwapAndBridge and compareShuttlingAndFlyingAncilla methods intentionally use division by zero to produce infinity when distance reductions are zero or negative. This infinity value marks that option as highly unfavorable in cost comparisons, which is the intended behavior rather than a bug.
Applied to files:
UPGRADING.mdinclude/hybridmap/HybridNeutralAtomMapper.hpp
📚 Learning: 2025-11-27T08:52:06.428Z
Learnt from: lsschmid
Repo: munich-quantum-toolkit/qmap PR: 832
File: src/hybridmap/HybridNeutralAtomMapper.cpp:1379-1383
Timestamp: 2025-11-27T08:52:06.428Z
Learning: In src/hybridmap/HybridNeutralAtomMapper.cpp, the getBestMovePos function throws std::runtime_error when no move position is found (finalBestPos.coords is empty). This behavior is acceptable for current use cases since tight architectures are not a concern. A future improvement would be to fall back to SWAP-based strategies instead of throwing.
Applied to files:
include/hybridmap/HybridNeutralAtomMapper.hpp
📚 Learning: 2025-11-05T07:42:45.507Z
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1287
File: test/qdmi/dd/error_handling_test.cpp:118-194
Timestamp: 2025-11-05T07:42:45.507Z
Learning: In the munich-quantum-toolkit/core QDMI device API, session parameters can only be set before calling `device_session_init()`. Once a session is initialized, any attempt to set a parameter returns `QDMI_ERROR_BADSTATE`. Since `SessionGuard` (in test/qdmi/dd/helpers/test_utils.hpp) automatically initializes the session in its constructor, tests that need to verify session parameter setting behavior before initialization must allocate a separate uninitialized session rather than reusing the `SessionGuard`'s session.
Applied to files:
include/hybridmap/HybridNeutralAtomMapper.hpp
📚 Learning: 2025-11-24T10:19:41.147Z
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1326
File: python/mqt/core/__init__.py:22-22
Timestamp: 2025-11-24T10:19:41.147Z
Learning: In the munich-quantum-toolkit/core repository, Ruff is configured with 'ALL' rules enabled by default, and only specific rules are selectively disabled. When reviewing changes that enable previously-disabled rules (like PLC0415), noqa directives for those rules become necessary and should be retained.
Applied to files:
include/hybridmap/HybridNeutralAtomMapper.hpp
📚 Learning: 2025-10-07T15:30:42.946Z
Learnt from: MatthiasReumann
Repo: munich-quantum-toolkit/core PR: 1237
File: mlir/include/mlir/Dialect/MQTOpt/Transforms/Transpilation/Layout.h:219-231
Timestamp: 2025-10-07T15:30:42.946Z
Learning: In the Layout class for MLIR quantum routing (mlir/include/mlir/Dialect/MQTOpt/Transforms/Transpilation/Layout.h), the swap method intentionally does NOT swap the hw fields in QubitInfo. This is correct because SSA values represent quantum states at fixed hardware locations, and only their program index associations change during a SWAP gate. The hw field indicates where an SSA value physically resides and remains constant.
Applied to files:
include/hybridmap/HybridNeutralAtomMapper.hpp
📚 Learning: 2025-10-09T04:30:29.071Z
Learnt from: MatthiasReumann
Repo: munich-quantum-toolkit/core PR: 1237
File: mlir/include/mlir/Dialect/MQTOpt/Transforms/Transpilation/Scheduler.h:72-140
Timestamp: 2025-10-09T04:30:29.071Z
Learning: In the CrawlScheduler (mlir/include/mlir/Dialect/MQTOpt/Transforms/Transpilation/Scheduler.h), the persistent `layoutCopy` is updated across all lookahead layers via `remapQubitValue` calls. After each two-qubit gate is scheduled in a layer, its input qubits are remapped to output qubits, ensuring the next layer's crawl starts from those outputs. This progressive remapping through SSA values prevents the same operation from being scheduled in multiple lookahead layers.
Applied to files:
include/hybridmap/HybridNeutralAtomMapper.hpp
🧬 Code graph analysis (1)
include/hybridmap/HybridNeutralAtomMapper.hpp (2)
include/hybridmap/HardwareQubits.hpp (7)
op(215-220)op(215-215)qubits(332-332)qubits(363-363)HardwareQubits(88-88)HardwareQubits(100-143)HardwareQubits(100-103)src/hybridmap/HybridNeutralAtomMapper.cpp (4)
mapAppend(45-123)mapAppend(45-46)convertToAod(168-183)convertToAod(168-168)
🪛 LanguageTool
docs/na_hybrid.md
[style] ~20-~20: The abbreviation “ASAP” (= as soon as possible) can sound harsh. Consider an alternative.
Context: ... capability-specific cost models and an ASAP scheduler that respects hardware constr...
(ASAP)
[style] ~228-~228: Consider using the typographical ellipsis character here instead.
Context: ...ile("path/to/circuit.qasm")and thenmapper.schedule(...)` as shown above. - Architectures: the...
(ELLIPSIS)
🪛 markdownlint-cli2 (0.18.1)
docs/na_hybrid.md
8-8: First line in a file should be a top-level heading
(MD041, first-line-heading, first-line-h1)
🔇 Additional comments (11)
UPGRADING.md (1)
17-22: Content and structure are clear.The documentation effectively describes the HyRoNA extension, new parameters, backwards compatibility, and optional .naviz output. The messaging aligns well with the PR objectives (issue #466).
include/hybridmap/HybridNeutralAtomMapper.hpp (10)
44-60: LGTM! MapperParameters structure is well-defined.The runtime configuration parameters are clearly documented with sensible defaults. The previously flagged documentation mismatch for
limitShuttlingLayerhas been resolved.
65-71: LGTM! Clean statistics structure.The
MapperStatsstruct provides clear, documented counters for tracking mapping operations.
87-126: Excellent! Parameters now stored by value.The refactor to store
MapperParameters parametersby value (line 109) completely resolves the critical dangling pointer issues raised in previous reviews. This makes the API much safer for C++ and Python users.Based on learnings, previous pointer-based storage allowed temporaries to dangle.
447-472: Excellent fixes! All critical constructor issues resolved.The constructor implementation now correctly addresses all previously raised critical concerns:
- Default constructor deleted (line 447) prevents invalid mapper state.
- Parameters stored by value via copy construction (line 450) eliminates dangling pointer risks.
- Signed arithmetic for validation (lines 454-455) avoids unsigned wrap bugs.
- Flying ancilla validation enforced (lines 461-462).
- Delegating constructor (line 472) safely copies parameters.
This is a robust, safe implementation.
480-493: Excellent! setParameters now safe and complete.The method correctly:
- Copies parameters by value (line 481) instead of storing a pointer.
- Validates constraints using signed arithmetic (lines 482-487).
- Checks flying ancilla limit (lines 489-491), which was missing in earlier versions.
All previously raised critical and major issues are resolved.
500-522: LGTM! State management methods are now complete and safe.Both
copyStateFromandresetare correctly implemented:
copyStateFromnow includesflyingAncillasassignment (line 509), which was previously missing.resetsafely accessesparametersmember (lines 518-521) without null dereference risk.Based on learnings, these fixes address previously identified gaps.
549-571: LGTM! Map methods are clean and efficient.The
mapoverload (lines 558-563) no longer contains the redundant DAG construction that was flagged in previous reviews. The implementation efficiently delegates to the main mapping method.
594-596: Excellent optimization! Return by const reference.Changing
getMappedQc()to return by const reference (line 594) avoids expensive deep copies of large circuits, as recommended in previous reviews.
634-643: Good! File I/O error handling implemented.The file open validation (lines 639-641) matches the documentation and provides clear error messages, as requested in previous reviews.
654-679: LGTM! Scheduling and animation methods are well-structured.The
schedulemethod correctly:
- Performs lazy AOD conversion (lines 657-659).
- Passes initial positions for both hardware qubits and flying ancillas (lines 660-662).
- Exposes animation artifacts consistently (lines 669-679).
|
@burgholzer I incorporated the suggested changes and fixed some more minor typos as suggested by CodeRabbit. I also added an entry to the Upgrading and ChangeLog file. |
burgholzer
left a comment
burgholzer
left a comment
There was a problem hiding this comment.
Nicely done 🙌🏼
Let's get this in! 🚀
I am waiting for Yannick's NA compiler changes to be merged before the next minor release. Should be soon.
Description
Thir PR extends the Hybrid Mapper within QMAP. In particular in includes the following features:
.navizand.namachinewhich can then be fed into NaVizFixes #466
Checklist: