✨ [NA] Add IDS Placement Method #862
Conversation
Co-authored-by: Copilot <[email protected]> Signed-off-by: Yannick Stade <[email protected]>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
include/na/zoned/layout_synthesizer/placer/HeuristicPlacer.hpp(17 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 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/na/zoned/layout_synthesizer/placer/HeuristicPlacer.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:
include/na/zoned/layout_synthesizer/placer/HeuristicPlacer.hpp
📚 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:
include/na/zoned/layout_synthesizer/placer/HeuristicPlacer.hpp
⏰ 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). (7)
- GitHub Check: 🇨 Test 🏁 (windows-2022, msvc, Release) / 🏁 windows-2022 msvc Release
- GitHub Check: 🐍 Test (macos-15-intel) / 🐍 macos-15-intel
- GitHub Check: 🐍 Test (macos-14) / 🐍 macos-14
- GitHub Check: 🐍 Test (ubuntu-24.04-arm) / 🐍 ubuntu-24.04-arm
- GitHub Check: 🐍 Test (windows-2022) / 🐍 windows-2022
- GitHub Check: 🐍 Test (ubuntu-24.04) / 🐍 ubuntu-24.04
- GitHub Check: 🇨 Test 🏁 (windows-2022, msvc, Debug) / 🏁 windows-2022 msvc Debug
🔇 Additional comments (7)
include/na/zoned/layout_synthesizer/placer/HeuristicPlacer.hpp (7)
27-28: LGTM! New includes and Method enum are appropriate.The added includes support the new IDS implementation (queue for BoundedPriorityQueue, spdlog for trace logging), and the Method enum clearly distinguishes between ASTAR and IDS algorithms.
Also applies to: 110-118
348-584: LGTM! BoundedPriorityQueue implementation is sound.The dual-heap structure (min-heap for priority order, max-heap for eviction) is correctly implemented with proper heapify operations maintaining both heap properties. The zero-capacity no-op behavior is documented, and the use of a
compare_member avoids repeated comparator construction. The complexity is justified for the bounded queue semantics needed by IDS.
586-683: LGTM! IDS implementation is correct and well-validated.The function properly validates
trials >= 1upfront (preventing underflow), correctly implements the iterative diving search algorithm with bounded queue and multiple trials, and provides clear error messages. The logic for restarting from the queue when goals are found or dead-ends are reached is sound.
174-191: LGTM! Factory methods elegantly handle method-specific defaults.The factory pattern successfully addresses the initialization-order fragility noted in past reviews. Method-specific defaults are now explicit and deterministic, and the JSON serialization correctly includes all configuration fields.
262-262: LGTM! Node lifetime and size optimizations are appropriate.Changing parent pointers to
std::shared_ptr<const Node>enables proper shared ownership in the search tree without circular reference risks (parent links only go upward). Usinguint16_tfor level reduces memory footprint and is justified for placement problems—a limit of 65,535 levels is far beyond practical atom/gate counts per layer.Also applies to: 267-267, 300-300, 305-305
1025-1054: LGTM! Updated getNeighbors signatures are consistent with shared_ptr-based nodes.The signatures correctly accept job vectors and
shared_ptr<const Node>parameters, returning vectors ofshared_ptr<const Node>. This is consistent with the new node lifetime model and enables proper ownership semantics throughout the search tree.
1122-1127: LGTM! JSON serialization for Method enum is correctly defined.The
NLOHMANN_JSON_SERIALIZE_ENUMmacro properly maps the Method enum to lowercase string values ("astar", "ids"), following JSON conventions and covering all enum values.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
include/na/zoned/layout_synthesizer/placer/HeuristicPlacer.hpp (1)
73-75: Fix outdated/typo’d Config docs (“A placer”, “@detsils”).*
Config is no longer A*-only, and the typo reduces doc quality.- /// The configuration of the A* placer + /// The configuration of the heuristic placer @@ - * @detsils If this number is exceeded, the search is aborted and an error + * @details If this number is exceeded, the search is aborted and an error * is raised.Also applies to: 155-159
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
include/na/zoned/layout_synthesizer/placer/HeuristicPlacer.hpp(17 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 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/na/zoned/layout_synthesizer/placer/HeuristicPlacer.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:
include/na/zoned/layout_synthesizer/placer/HeuristicPlacer.hpp
📚 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:
include/na/zoned/layout_synthesizer/placer/HeuristicPlacer.hpp
🧬 Code graph analysis (1)
include/na/zoned/layout_synthesizer/placer/HeuristicPlacer.hpp (1)
src/na/zoned/layout_synthesizer/placer/HeuristicPlacer.cpp (16)
getNeighbors(1215-1249)getNeighbors(1215-1217)getNeighbors(1251-1294)getNeighbors(1251-1253)isGoal(41-44)isGoal(41-42)isGoal(45-48)isGoal(45-46)getCost(1084-1090)getCost(1084-1084)getCost(1092-1098)getCost(1092-1092)getHeuristic(1127-1170)getHeuristic(1127-1131)getHeuristic(1172-1213)getHeuristic(1172-1176)
⏰ 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). (6)
- GitHub Check: 🐍 Test (ubuntu-24.04) / 🐍 ubuntu-24.04
- GitHub Check: 🐍 Test (macos-14) / 🐍 macos-14
- GitHub Check: 🐍 Test (ubuntu-24.04-arm) / 🐍 ubuntu-24.04-arm
- GitHub Check: 🐍 Test (macos-15-intel) / 🐍 macos-15-intel
- GitHub Check: 🐍 Test (windows-2022) / 🐍 windows-2022
- GitHub Check: 🇨 Test 🏁 (windows-2022, msvc, Debug) / 🏁 windows-2022 msvc Debug
🔇 Additional comments (2)
include/na/zoned/layout_synthesizer/placer/HeuristicPlacer.hpp (2)
258-266: Validate/assumelevelfits intouint16_t(or switch tosize_t).
levelis used to index job vectors (via casts in the .cpp). If job counts can exceed 65,535 in any realistic scenario, this will overflow silently; otherwise, please codify the precondition (assert/throw at setup time).struct AtomNode { @@ - uint16_t level = 0; + uint16_t level = 0; // Precondition: number of placement steps <= 65535 @@ struct GateNode { @@ - uint16_t level = 0; + uint16_t level = 0; // Precondition: number of placement steps <= 65535Also applies to: 296-304
111-118: Method enum + JSON mapping looks consistent with defaults/factory approach.
Method is serialized as"astar"/"ids"and Config includes it in the NLOHMANN default macro; this should keep Python/C++ config round-tripping stable.Also applies to: 186-190, 1121-1126
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
include/na/zoned/layout_synthesizer/placer/HeuristicPlacer.hpp (1)
73-73: Update outdated documentation reference.The comment refers to "A* placer" but the class has been renamed to
HeuristicPlacerand now supports both A* and IDS methods.Apply this diff:
- /// The configuration of the A* placer + /// The configuration of the heuristic placer struct Config {Also applies to: 194-194, 330-330
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
include/na/zoned/layout_synthesizer/placer/HeuristicPlacer.hpp(17 hunks)pyproject.toml(3 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 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/na/zoned/layout_synthesizer/placer/HeuristicPlacer.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:
include/na/zoned/layout_synthesizer/placer/HeuristicPlacer.hpp
📚 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:
include/na/zoned/layout_synthesizer/placer/HeuristicPlacer.hpp
⏰ 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). (1)
- GitHub Check: 🐍 Test (macos-15-intel) / 🐍 macos-15-intel
🔇 Additional comments (5)
include/na/zoned/layout_synthesizer/placer/HeuristicPlacer.hpp (5)
74-190: LGTM! Config structure with factory methods looks solid.The factory method approach properly addresses the method-dependent defaults concern from previous reviews. The Method enum, new parameters (trials, queueCapacity), and JSON serialization are all correctly implemented.
346-593: LGTM! BoundedPriorityQueue implementation is robust.The previous review concerns about heap invariants, documentation, and edge cases have all been properly addressed. The dual-heap approach with proper up-or-down heapify logic in
pop()(lines 534-545) correctly maintains both min and max heap properties.
595-695: LGTM! Iterative diving search implementation is solid.All concerns from previous reviews have been addressed:
- Precondition validation for
trials >= 1(lines 623-625)- Safe handling of
std::optional<Item> goal- Clear documentation of start node cost requirement (lines 603-604)
- Proper queue capacity initialization with explanatory comment (lines 640-642)
697-804: LGTM! A tree search implementation properly updated.*The function has been correctly modernized with:
- Shared pointer-based node types for proper lifetime management
maxNodes > 0validation (lines 745-747)- Documentation of start node cost requirement (lines 727-728)
- Proper error handling with helpful messages
All previous review concerns have been addressed.
258-326: LGTM! Node structure modernization with shared_ptr is well-designed.The migration to
shared_ptr<const Node>parent pointers provides:
- Proper lifetime management without manual memory handling
- Const correctness ensuring nodes are immutable after creation
- Safe sharing of parent references across the search tree
The widening of
levelfromuint8_ttouint16_t(lines 265, 303) appropriately supports deeper search trees for more complex placement problems.Also applies to: 1038-1062
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (1)
pyproject.toml(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-13T00:03:08.078Z
Learnt from: denialhaag
Repo: munich-quantum-toolkit/debugger PR: 160
File: pyproject.toml:252-256
Timestamp: 2025-10-13T00:03:08.078Z
Learning: In uv's `override-dependencies`, multiple entries for the same package with different environment markers (or one without a marker and one with a specific marker) can coexist. uv correctly resolves these by applying the appropriate constraint based on the active Python version. For example, `["symengine>=0.11,<0.14", "symengine>=0.14.1; python_version >= '3.14'"]` is valid and will apply the second constraint on Python 3.14+.
Applied to files:
pyproject.toml
⏰ 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). (6)
- GitHub Check: 🐍 Test (windows-2022) / 🐍 windows-2022
- GitHub Check: 🇨 Test 🏁 (windows-2022, msvc, Debug) / 🏁 windows-2022 msvc Debug
- GitHub Check: 🐍 Test (ubuntu-24.04) / 🐍 ubuntu-24.04
- GitHub Check: 🐍 Test (macos-14) / 🐍 macos-14
- GitHub Check: 🐍 Test (macos-15-intel) / 🐍 macos-15-intel
- GitHub Check: 🐍 Test (ubuntu-24.04-arm) / 🐍 ubuntu-24.04-arm
Signed-off-by: burgholzer <[email protected]>
Signed-off-by: burgholzer <[email protected]>
## Description ### Summary by CodeRabbit #### New Features - Introduced PlacementMethod configuration enum with astar and ids placement algorithm options - Added placement_method parameter to RoutingAwareCompiler for flexible algorithm selection during compilation #### Tests - Extended test coverage with new placement configuration scenarios and comprehensive test cases ### Walkthrough This PR refactors the quantum placement system by replacing AStarPlacer with HeuristicPlacer, introducing a configurable placement method enum (ASTAR and IDS) exposed to Python, modernizing method signatures to use shared_ptr-based nodes, and updating the placer configuration pipeline to accept placement method parameters in both C++ and Python interfaces. ## Checklist: <!--- This checklist serves as a reminder of a couple of things that ensure your pull request will be merged swiftly. --> - [x] The pull request only contains commits that are focused and relevant to this change. - [x] I have added appropriate tests that cover the new/changed functionality. - [x] I have updated the documentation to reflect these changes. - [x] I have added entries to the changelog for any noteworthy additions, changes, fixes, or removals. - [ ] I have added migration instructions to the upgrade guide (if needed). - [x] The changes follow the project's style guidelines and introduce no new warnings. - [x] The changes are fully tested and pass the CI checks. - [x] I have reviewed my own code changes. --------- Signed-off-by: Yannick Stade <[email protected]> Co-authored-by: Copilot <[email protected]> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Daniel Haag <[email protected]> # Conflicts: # include/na/zoned/layout_synthesizer/placer/HeuristicPlacer.hpp
Description
Summary by CodeRabbit
New Features
Tests
Walkthrough
This PR refactors the quantum placement system by replacing AStarPlacer with HeuristicPlacer, introducing a configurable placement method enum (ASTAR and IDS) exposed to Python, modernizing method signatures to use shared_ptr-based nodes, and updating the placer configuration pipeline to accept placement method parameters in both C++ and Python interfaces.
Checklist: