✨ New QDMI Device to represent SC Devices #1328
Conversation
ystade
added
feature
|
I am aware of the code duplicates between the NA and SC device originating from a lot of copy paste. This PR is meant to provide a first initial implementation for @MatthiasReumann to test his QMAP implementation with a QDMI device. I do plan to refactor this initial implementation at a later point. If ok, I would like to merge this initial PR rather quickly and simultaneously open a tracking issue to polish this initial implementation. |
|
While working on this PR, it might also an idea to move the NA QDMI device to the QDMI directory, @burgholzer ? Should I open an issue for that as well? |
📝 WalkthroughSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughMoved NA Device into namespace qdmi::na and converted several NA job methods to instance methods. Added a full SC QDMI backend (generator, CLI, static/dynamic libraries, C API, header generation from a 100‑qubit JSON), integrated SC into Driver/CMake, and added tests and test CMake updates. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant Driver
participant SC_Device as "MQT_SC Device (singleton)"
participant Session
participant Job
Client->>Driver: load/register device libraries
Driver->>SC_Device: register static SC library
Client->>SC_Device: initialize()
Client->>SC_Device: sessionAlloc() → Session
Client->>Session: init() / setParameter(...)
Client->>Session: createDeviceJob() → Job
Client->>Job: setParameter(...), submit()
Job->>SC_Device: submit processed (NOTSUPPORTED or queued)
Client->>Job: check()/wait()/get_results()
Client->>SC_Device: sessionFree(Session)
Client->>SC_Device: finalize()
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (6)📓 Common learnings📚 Learning: 2025-11-26T10:29:36.435ZApplied to files:
📚 Learning: 2025-11-04T14:28:32.371ZApplied to files:
📚 Learning: 2025-10-09T13:20:11.483ZApplied to files:
📚 Learning: 2025-11-01T15:57:31.153ZApplied to files:
📚 Learning: 2025-10-17T11:09:50.147ZApplied to files:
🧬 Code graph analysis (1)test/fomac/test_fomac.cpp (3)
⏰ 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). (4)
🔇 Additional comments (8)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (19)
include/mqt-core/na/device/Device.hpp(2 hunks)include/mqt-core/qdmi/Driver.hpp(1 hunks)include/mqt-core/qdmi/sc/Device.hpp(1 hunks)include/mqt-core/qdmi/sc/Generator.hpp(1 hunks)json/sc/device.json(1 hunks)src/na/device/Device.cpp(5 hunks)src/qdmi/CMakeLists.txt(2 hunks)src/qdmi/Driver.cpp(3 hunks)src/qdmi/sc/App.cpp(1 hunks)src/qdmi/sc/CMakeLists.txt(1 hunks)src/qdmi/sc/Device.cpp(1 hunks)src/qdmi/sc/DynDevice.cpp(1 hunks)src/qdmi/sc/Generator.cpp(1 hunks)test/qdmi/CMakeLists.txt(1 hunks)test/qdmi/sc/CMakeLists.txt(1 hunks)test/qdmi/sc/test_app.cpp(1 hunks)test/qdmi/sc/test_device.cpp(1 hunks)test/qdmi/sc/test_generator.cpp(1 hunks)test/qdmi/test_driver.cpp(2 hunks)
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: marcelwa
Repo: munich-quantum-toolkit/core PR: 1243
File: test/python/qdmi/qiskit/test_qdmi_qiskit_backend.py:0-0
Timestamp: 2025-11-04T14:28:32.371Z
Learning: In the munich-quantum-toolkit/core repository, at least one FoMaC device is always available during testing, so skip logic for missing devices in QDMI Qiskit backend tests is not necessary.
Learnt from: marcelwa
Repo: munich-quantum-toolkit/core PR: 1243
File: test/python/qdmi/qiskit/test_qdmi_qiskit_site_handling.py:162-173
Timestamp: 2025-10-17T11:09:50.147Z
Learning: For QDMI tests that reference a specific named device via fixture (e.g., "MQT NA Default QDMI Device"), it is acceptable to hard-code device-specific values such as zone counts and indices, as these tests are intentionally coupled to that device's structure and serve as structural invariant checks.
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1287
File: test/qdmi/dd/CMakeLists.txt:9-21
Timestamp: 2025-11-03T23:09:26.881Z
Learning: The CMake functions `generate_device_defs_executable` and `generate_prefixed_qdmi_headers` used in QDMI device test CMakeLists.txt files are provided by the external QDMI library (fetched via FetchContent from https://github.com/Munich-Quantum-Software-Stack/qdmi.git), specifically in the cmake/PrefixHandling.cmake module of the QDMI repository.
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.
📚 Learning: 2025-10-17T11:09:50.147Z
Learnt from: marcelwa
Repo: munich-quantum-toolkit/core PR: 1243
File: test/python/qdmi/qiskit/test_qdmi_qiskit_site_handling.py:162-173
Timestamp: 2025-10-17T11:09:50.147Z
Learning: For QDMI tests that reference a specific named device via fixture (e.g., "MQT NA Default QDMI Device"), it is acceptable to hard-code device-specific values such as zone counts and indices, as these tests are intentionally coupled to that device's structure and serve as structural invariant checks.
Applied to files:
json/sc/device.jsontest/qdmi/sc/CMakeLists.txttest/qdmi/CMakeLists.txtinclude/mqt-core/na/device/Device.hppsrc/qdmi/sc/CMakeLists.txtsrc/qdmi/Driver.cppsrc/na/device/Device.cpptest/qdmi/sc/test_device.cpptest/qdmi/test_driver.cppinclude/mqt-core/qdmi/sc/Device.hppsrc/qdmi/sc/Device.cppsrc/qdmi/sc/DynDevice.cpp
📚 Learning: 2025-11-04T14:28:32.371Z
Learnt from: marcelwa
Repo: munich-quantum-toolkit/core PR: 1243
File: test/python/qdmi/qiskit/test_qdmi_qiskit_backend.py:0-0
Timestamp: 2025-11-04T14:28:32.371Z
Learning: In the munich-quantum-toolkit/core repository, at least one FoMaC device is always available during testing, so skip logic for missing devices in QDMI Qiskit backend tests is not necessary.
Applied to files:
json/sc/device.jsontest/qdmi/sc/CMakeLists.txtsrc/qdmi/Driver.cpptest/qdmi/sc/test_device.cpptest/qdmi/test_driver.cppinclude/mqt-core/qdmi/sc/Device.hpp
📚 Learning: 2025-11-03T23:09:26.881Z
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1287
File: test/qdmi/dd/CMakeLists.txt:9-21
Timestamp: 2025-11-03T23:09:26.881Z
Learning: The CMake functions `generate_device_defs_executable` and `generate_prefixed_qdmi_headers` used in QDMI device test CMakeLists.txt files are provided by the external QDMI library (fetched via FetchContent from https://github.com/Munich-Quantum-Software-Stack/qdmi.git), specifically in the cmake/PrefixHandling.cmake module of the QDMI repository.
Applied to files:
include/mqt-core/qdmi/sc/Generator.hpptest/qdmi/sc/CMakeLists.txttest/qdmi/CMakeLists.txtsrc/qdmi/sc/Generator.cppsrc/qdmi/sc/CMakeLists.txtsrc/qdmi/CMakeLists.txtsrc/qdmi/Driver.cpptest/qdmi/sc/test_device.cpptest/qdmi/test_driver.cppinclude/mqt-core/qdmi/sc/Device.hppsrc/qdmi/sc/Device.cppsrc/qdmi/sc/DynDevice.cpp
📚 Learning: 2025-10-10T08:09:54.528Z
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1246
File: bindings/CMakeLists.txt:0-0
Timestamp: 2025-10-10T08:09:54.528Z
Learning: In the Munich Quantum Toolkit (MQT) Core project, scikit-build-core is configured with `wheel.install-dir = "mqt/core"` in pyproject.toml, which automatically prefixes all CMake `DESTINATION` paths with `mqt/core/` during wheel installation. Therefore, CMake install destinations are relative to the `mqt/core` package namespace, not `site-packages`.
Applied to files:
test/qdmi/CMakeLists.txtsrc/qdmi/sc/CMakeLists.txtsrc/qdmi/CMakeLists.txt
📚 Learning: 2025-11-04T19:50:14.489Z
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1287
File: test/qdmi/dd/helpers/test_utils.cpp:103-118
Timestamp: 2025-11-04T19:50:14.489Z
Learning: In the munich-quantum-toolkit/core QDMI device tests, when QDMI APIs return a string result size `ks` that includes a null terminator, the pattern `std::string keys(ks - 1, '\0'); ... device_job_get_results(..., ks, keys.data(), nullptr)` is correct and safe. The string allocates `ks` bytes total (ks-1 content + implicit null), and the API writing `ks` bytes (including trailing null) fits exactly into this space without buffer overrun.
Applied to files:
include/mqt-core/na/device/Device.hppsrc/qdmi/Driver.cpptest/qdmi/sc/test_device.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:
include/mqt-core/na/device/Device.hppsrc/na/device/Device.cpptest/qdmi/sc/test_device.cpptest/qdmi/test_driver.cppinclude/mqt-core/qdmi/sc/Device.hppsrc/qdmi/sc/Device.cppsrc/qdmi/sc/DynDevice.cpp
🧬 Code graph analysis (6)
include/mqt-core/qdmi/sc/Generator.hpp (2)
include/mqt-core/qdmi/sc/Device.hpp (1)
nodiscard(58-61)src/qdmi/sc/Generator.cpp (2)
nodiscard(126-138)nodiscard(140-149)
src/qdmi/sc/Generator.cpp (2)
include/mqt-core/qdmi/sc/Device.hpp (2)
id(239-240)nodiscard(58-61)include/mqt-core/qdmi/sc/Generator.hpp (6)
writeJSONSchema(56-56)writeJSONSchema(65-65)readJSON(74-74)readJSON(83-83)writeHeader(94-94)writeHeader(105-105)
test/qdmi/sc/test_app.cpp (2)
test/qir/runtime/test_qir_runtime.cpp (1)
cout(37-37)test/qdmi/sc/test_device.cpp (1)
ASSERT_EQ(56-68)
src/qdmi/sc/App.cpp (2)
include/mqt-core/qdmi/sc/Generator.hpp (6)
writeJSONSchema(56-56)writeJSONSchema(65-65)readJSON(74-74)readJSON(83-83)writeHeader(94-94)writeHeader(105-105)src/qdmi/sc/Generator.cpp (10)
writeJSONSchema(98-111)writeJSONSchema(98-98)writeJSONSchema(113-124)writeJSONSchema(113-113)readJSON(126-126)readJSON(140-140)writeHeader(151-157)writeHeader(151-151)writeHeader(159-169)writeHeader(159-159)
include/mqt-core/qdmi/sc/Device.hpp (1)
include/mqt-core/na/device/Device.hpp (8)
Device(69-69)Device(73-73)Device(76-76)Device(86-86)session(92-92)session(98-98)id(280-283)id(285-287)
src/qdmi/sc/DynDevice.cpp (1)
include/mqt-core/qdmi/sc/Device.hpp (13)
session(70-70)session(76-76)param(115-116)param(183-184)job(122-122)job(128-128)prop(82-83)prop(134-135)prop(190-191)prop(245-246)site(141-143)operation(149-153)numSites(266-269)
🪛 GitHub Check: 🇨 Lint / 🚨 Lint
src/qdmi/sc/Generator.cpp
[warning] 33-33: src/qdmi/sc/Generator.cpp:33:1 [misc-include-cleaner]
included header utility is not used directly
[warning] 32-32: src/qdmi/sc/Generator.cpp:32:1 [misc-include-cleaner]
included header tuple is not used directly
[warning] 27-27: src/qdmi/sc/Generator.cpp:27:1 [misc-include-cleaner]
included header ranges is not used directly
[warning] 23-23: src/qdmi/sc/Generator.cpp:23:1 [misc-include-cleaner]
included header functional is not used directly
[warning] 20-20: src/qdmi/sc/Generator.cpp:20:1 [misc-include-cleaner]
included header cstddef is not used directly
[warning] 17-17: src/qdmi/sc/Generator.cpp:17:1 [misc-include-cleaner]
included header algorithm is not used directly
test/qdmi/sc/test_device.cpp
[warning] 12-12: test/qdmi/sc/test_device.cpp:12:1 [misc-include-cleaner]
included header Generator.hpp is not used directly
src/qdmi/sc/Device.cpp
[warning] 292-292: src/qdmi/sc/Device.cpp:292:36 [readability-convert-member-functions-to-static]
method 'queryProperty' can be made static
[warning] 290-290: src/qdmi/sc/Device.cpp:290:57 [performance-move-const-arg]
std::move of the variable 'op' of the trivially-copyable type 'MQT_SC_QDMI_Operation_impl_d' has no effect; remove std::move()
[warning] 20-20: src/qdmi/sc/Device.cpp:20:1 [misc-include-cleaner]
included header algorithm 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). (4)
- GitHub Check: 🐍 Test (macos-15-intel) / 🐍 macos-15-intel
- GitHub Check: 🐍 Test (macos-14) / 🐍 macos-14
- GitHub Check: 🐍 Test (windows-2022) / 🐍 windows-2022
- GitHub Check: 🇨 Test 🍎 (macos-15-intel, clang, Release) / 🍎 macos-15-intel clang Release
🔇 Additional comments (14)
json/sc/device.json (1)
1-186: Coupling map JSON is well-formed and index-safeThe
numQubitsandcouplingslist are consistent (indices stay within[0, 99]and form a regular 10×10 grid of nearest‑neighbor edges). No structural issues from the JSON side.include/mqt-core/qdmi/Driver.hpp (1)
141-144: SC static device library declaration aligns with existing patternAdding
DECLARE_STATIC_LIBRARY(MQT_SC)mirrors the NA and DDSIM declarations and cleanly extends the static backend list; just ensure the corresponding implementation (DEFINE_STATIC_LIBRARY(MQT_SC)or equivalent) is provided in the source so linkage succeeds.test/qdmi/sc/CMakeLists.txt (1)
9-31: SC QDMI test target wiring is consistent with existing device-test patternsThe SC device test setup (guarded on SC targets existing, reuse of
generate_device_defs_executable, dependency onqdmi_test_mqt_sc_device_defs, linking againstCoreQDMIScDeviceGenandnlohmann_json, and passingEXECUTABLE_PATH/SC_DEVICE_JSON) looks coherent and in line with the existing QDMI device test infrastructure.src/na/device/Device.cpp (1)
92-163: NA device implementation namespace move toqdmi::nais consistent and non-breakingWrapping
Deviceinnamespace qdmi::naand updating the few call sites (queryDeviceProperty,MQT_NA_QDMI_device_initialize,*_session_alloc/free) to useqdmi::na::Device::get()aligns the implementation with the header move without altering behavior. The global C API symbols remain unchanged.Also applies to: 210-211, 561-581
include/mqt-core/na/device/Device.hpp (1)
29-107: NADevicedeclaration correctly moved intoqdmi::nanamespacePlacing
Deviceinnamespace qdmi::na(with the rest of the neutral-atom specifics) matches the implementation and keeps the QDMI core namespace less crowded; the surrounding QDMI impl structs remain at global scope as before, so the C API surface stays stable.test/qdmi/sc/test_device.cpp (1)
52-348: SC device session/job coverage looks solidThe session/job lifecycle and property tests (including BADSTATE vs INVALIDARGUMENT vs NOTSUPPORTED cases) are consistent with the QDMI conventions and exercise the SC backend well.
src/qdmi/CMakeLists.txt (1)
10-10: SC device library is wired correctly into the QDMI driverAdding
add_subdirectory(sc)and linkingMQT::CoreQDMIScDeviceprivately into the driver aligns with the existing NA and DDSIM wiring and keeps the public interface unchanged.Also applies to: 29-30
src/qdmi/sc/App.cpp (1)
168-195: CLI parsing and command dispatch are well‑structuredThe separation between global parsing (
parseArguments), per‑command parsers, andexecute*handlers, plus the defensive try/catch and logging around both argv handling and subcommand parsing, looks clean and robust.Also applies to: 277-385, 399-446
src/qdmi/Driver.cpp (1)
15-15: SC static device library integration matches existing patternIncluding
mqt_sc_qdmi/device.h, definingDEFINE_STATIC_LIBRARY(MQT_SC), and adding anMQT_SCDeviceLibraryinstance in theDriverconstructor mirror the established NA/DDSIM setup and cleanly expose the new SC backend to QDMI sessions.Also applies to: 78-81, 335-343
src/qdmi/sc/CMakeLists.txt (1)
9-205: SC CMake wiring and header generation look consistent.Targets, aliases, and header‑generation/custom‑command wiring match the QDMI patterns (including
generate_prefixed_qdmi_headersand JSON‑driven header gen). Dependency ordering viaadd_dependencies(... generate_qdmi_sc_device_header)and versioning/export properties look good.include/mqt-core/qdmi/sc/Device.hpp (1)
24-270: SC device interface mirrors existing QDMI patterns cleanly.Singleton
Deviceand the*_impl_dhelper structs follow the established QDMI design (session/job/site/operation separation) and provide a clear, minimal public surface for SC devices. From the header alone, there are no obvious correctness or API‑shape issues.src/qdmi/sc/Device.cpp (3)
92-151: Device construction and device-level property dispatch look consistent and robustThe singleton
Deviceconstructor andqueryPropertyimplementation:
- Correctly initialize name, qubit count, sites, and coupling map via the initializer helpers.
- Validate arguments (
value != nullptr && size == 0, enum range checks).- Use the helper macros to return proper sizes and types for device properties, including sites, coupling map, and operations.
- Return
QDMI_ERROR_NOTSUPPORTEDfor unhandled but in-range properties, which aligns well with a minimal device.One thing to double-check outside this file is that
operations_is initialized consistently with the SC generator/config, but from this file’s perspective the plumbing is sound.
154-172: Session parameter state machine correctly enforces “only before init” rule
MQT_SC_QDMI_Device_Session_impl_d::initandsetParameter:
- Enforce that
init()is only valid in theALLOCATEDstate.- Enforce that
setParameteris only allowed whilestatus_ == Status::ALLOCATED, returningQDMI_ERROR_BADSTATEafterwards.- Validate arguments (
value != nullptr && size == 0, parameter enum range).This matches the documented QDMI behavior that session parameters can only be set before
device_session_init()is called.Based on learnings, this aligns with the expected session parameter semantics.
225-271: Guard against null job handles inMQT_SC_QDMI_device_job_freeMost job-related C API wrappers validate
job == nullptrand returnQDMI_ERROR_INVALIDARGUMENT, butMQT_SC_QDMI_device_job_freedereferences the handle unconditionally viajob->free(). This will segfault if a caller ever passes a null job (or a stale pointer):void MQT_SC_QDMI_device_job_free(MQT_SC_QDMI_Device_Job job) { job->free(); }Since the function is
voidand cannot report an error, turning a null handle into a no-op is the safest behavior and consistent withsessionFree:-void MQT_SC_QDMI_device_job_free(MQT_SC_QDMI_Device_Job job) { job->free(); } +void MQT_SC_QDMI_device_job_free(MQT_SC_QDMI_Device_Job job) { + if (job != nullptr) { + job->free(); + } +}This eliminates a potential null-pointer dereference in the public C API.
⛔ Skipped due to learnings
Learnt from: burgholzer Repo: munich-quantum-toolkit/core PR: 1287 File: test/qdmi/dd/helpers/test_utils.cpp:103-118 Timestamp: 2025-11-04T19:50:14.489Z Learning: In the munich-quantum-toolkit/core QDMI device tests, when QDMI APIs return a string result size `ks` that includes a null terminator, the pattern `std::string keys(ks - 1, '\0'); ... device_job_get_results(..., ks, keys.data(), nullptr)` is correct and safe. The string allocates `ks` bytes total (ks-1 content + implicit null), and the API writing `ks` bytes (including trailing null) fits exactly into this space without buffer overrun.Learnt from: marcelwa Repo: munich-quantum-toolkit/core PR: 1243 File: test/python/qdmi/qiskit/test_qdmi_qiskit_site_handling.py:162-173 Timestamp: 2025-10-17T11:09:50.147Z Learning: For QDMI tests that reference a specific named device via fixture (e.g., "MQT NA Default QDMI Device"), it is acceptable to hard-code device-specific values such as zone counts and indices, as these tests are intentionally coupled to that device's structure and serve as structural invariant checks.
|
Note Docstrings generation - SUCCESS |
Docstrings generation was requested by @ystade. * #1328 (comment) The following files were modified: * `include/mqt-core/qdmi/sc/Device.hpp` * `src/na/device/Device.cpp` * `src/qdmi/Driver.cpp` * `src/qdmi/sc/App.cpp` * `src/qdmi/sc/Device.cpp` * `src/qdmi/sc/DynDevice.cpp` * `src/qdmi/sc/Generator.cpp` * `test/na/device/test_app.cpp` * `test/na/device/test_device.cpp` * `test/na/device/test_generator.cpp` * `test/qdmi/sc/test_app.cpp` * `test/qdmi/sc/test_device.cpp` * `test/qdmi/sc/test_generator.cpp` * `test/qdmi/test_driver.cpp`
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
I am fine with moving rather quickly here. |
Docstrings generation was requested by @ystade. * #1328 (comment) The following files were modified: * `include/mqt-core/qdmi/sc/Device.hpp` * `src/na/device/Device.cpp` * `src/qdmi/Driver.cpp` * `src/qdmi/sc/App.cpp` * `src/qdmi/sc/Device.cpp` * `src/qdmi/sc/DynDevice.cpp` * `src/qdmi/sc/Generator.cpp` * `test/na/device/test_app.cpp` * `test/na/device/test_device.cpp` * `test/na/device/test_generator.cpp` * `test/qdmi/sc/test_app.cpp` * `test/qdmi/sc/test_device.cpp` * `test/qdmi/sc/test_generator.cpp` * `test/qdmi/test_driver.cpp` <details> <summary>These file types are not supported</summary> * `json/sc/device.json` * `src/qdmi/CMakeLists.txt` * `src/qdmi/sc/CMakeLists.txt` * `test/qdmi/CMakeLists.txt` * `test/qdmi/sc/CMakeLists.txt` </details> <details> <summary>ℹ️ Note</summary><blockquote> CodeRabbit cannot perform edits on its own pull requests yet. </blockquote></details> Co-authored-by: Yannick Stade <100073938+ystade@users.noreply.github.com>
Yeah. I have thought about that as well. |
ystade
changed the title
ystade
added
enhancement
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (3)
src/qdmi/sc/App.cpp (1)
94-112: Fixgenerateusage string to match optionaljson_fileargumentThe usage line shows
<json_file>as required, but the description and implementation treat it as optional (stdin fallback). The argument should be shown in square brackets[<json_file>]to match the behavior.src/qdmi/sc/Generator.cpp (2)
11-13: Update @file description to refer to superconducting devices.The file comment says "neutral atom devices" but this file is for SC devices. Should say "superconducting devices" or "SC devices".
17-35: Remove unused includes to satisfy misc-include-cleaner.Static analysis flags several unused headers:
algorithm,cstddef,functional,ranges,tuple, andutility. Removing them would silence warnings and improve build times.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
include/mqt-core/qdmi/sc/Device.hpp(1 hunks)src/qdmi/sc/App.cpp(1 hunks)src/qdmi/sc/Generator.cpp(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: marcelwa
Repo: munich-quantum-toolkit/core PR: 1243
File: test/python/qdmi/qiskit/test_qdmi_qiskit_backend.py:0-0
Timestamp: 2025-11-04T14:28:32.371Z
Learning: In the munich-quantum-toolkit/core repository, at least one FoMaC device is always available during testing, so skip logic for missing devices in QDMI Qiskit backend tests is not necessary.
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1287
File: test/qdmi/dd/CMakeLists.txt:9-21
Timestamp: 2025-11-03T23:09:26.881Z
Learning: The CMake functions `generate_device_defs_executable` and `generate_prefixed_qdmi_headers` used in QDMI device test CMakeLists.txt files are provided by the external QDMI library (fetched via FetchContent from https://github.com/Munich-Quantum-Software-Stack/qdmi.git), specifically in the cmake/PrefixHandling.cmake module of the QDMI repository.
📚 Learning: 2025-11-03T23:09:26.881Z
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1287
File: test/qdmi/dd/CMakeLists.txt:9-21
Timestamp: 2025-11-03T23:09:26.881Z
Learning: The CMake functions `generate_device_defs_executable` and `generate_prefixed_qdmi_headers` used in QDMI device test CMakeLists.txt files are provided by the external QDMI library (fetched via FetchContent from https://github.com/Munich-Quantum-Software-Stack/qdmi.git), specifically in the cmake/PrefixHandling.cmake module of the QDMI repository.
Applied to files:
src/qdmi/sc/Generator.cppinclude/mqt-core/qdmi/sc/Device.hpp
📚 Learning: 2025-10-17T11:09:50.147Z
Learnt from: marcelwa
Repo: munich-quantum-toolkit/core PR: 1243
File: test/python/qdmi/qiskit/test_qdmi_qiskit_site_handling.py:162-173
Timestamp: 2025-10-17T11:09:50.147Z
Learning: For QDMI tests that reference a specific named device via fixture (e.g., "MQT NA Default QDMI Device"), it is acceptable to hard-code device-specific values such as zone counts and indices, as these tests are intentionally coupled to that device's structure and serve as structural invariant checks.
Applied to files:
include/mqt-core/qdmi/sc/Device.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/mqt-core/qdmi/sc/Device.hpp
📚 Learning: 2025-11-04T14:28:32.371Z
Learnt from: marcelwa
Repo: munich-quantum-toolkit/core PR: 1243
File: test/python/qdmi/qiskit/test_qdmi_qiskit_backend.py:0-0
Timestamp: 2025-11-04T14:28:32.371Z
Learning: In the munich-quantum-toolkit/core repository, at least one FoMaC device is always available during testing, so skip logic for missing devices in QDMI Qiskit backend tests is not necessary.
Applied to files:
include/mqt-core/qdmi/sc/Device.hpp
🧬 Code graph analysis (3)
src/qdmi/sc/Generator.cpp (2)
include/mqt-core/qdmi/sc/Device.hpp (2)
id(252-253)nodiscard(62-65)include/mqt-core/qdmi/sc/Generator.hpp (6)
writeJSONSchema(56-56)writeJSONSchema(65-65)readJSON(74-74)readJSON(83-83)writeHeader(94-94)writeHeader(105-105)
src/qdmi/sc/App.cpp (2)
src/qdmi/sc/Generator.cpp (10)
writeJSONSchema(114-127)writeJSONSchema(114-114)writeJSONSchema(139-150)writeJSONSchema(139-139)readJSON(162-162)readJSON(185-185)writeHeader(207-213)writeHeader(207-207)writeHeader(225-235)writeHeader(225-225)include/mqt-core/qdmi/sc/Generator.hpp (6)
writeJSONSchema(56-56)writeJSONSchema(65-65)readJSON(74-74)readJSON(83-83)writeHeader(94-94)writeHeader(105-105)
include/mqt-core/qdmi/sc/Device.hpp (1)
include/mqt-core/qdmi/Driver.hpp (5)
session(424-424)session(430-430)prop(220-221)prop(283-284)prop(363-364)
🪛 GitHub Check: 🇨 Lint / 🚨 Lint
src/qdmi/sc/Generator.cpp
[warning] 33-33: src/qdmi/sc/Generator.cpp:33:1 [misc-include-cleaner]
included header utility is not used directly
[warning] 32-32: src/qdmi/sc/Generator.cpp:32:1 [misc-include-cleaner]
included header tuple is not used directly
[warning] 27-27: src/qdmi/sc/Generator.cpp:27:1 [misc-include-cleaner]
included header ranges is not used directly
[warning] 23-23: src/qdmi/sc/Generator.cpp:23:1 [misc-include-cleaner]
included header functional is not used directly
[warning] 20-20: src/qdmi/sc/Generator.cpp:20:1 [misc-include-cleaner]
included header cstddef is not used directly
[warning] 17-17: src/qdmi/sc/Generator.cpp:17:1 [misc-include-cleaner]
included header algorithm 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
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (3)
src/qdmi/sc/App.cpp (1)
94-112: Fixgenerateusage string to match optionaljson_fileargument.The usage line shows
<json_file>as required, but both the description (lines 102-104) andparseGenerateArguments(lines 275-291) treat it as optional with stdin fallback.Apply this diff to mark the argument as optional:
"Usage: " << programName - << " generate [options] <json_file>\n" + << " generate [options] [<json_file>]\n"src/qdmi/sc/Generator.cpp (2)
11-13: Update file description to refer to superconducting devices.The file comment incorrectly refers to "neutral atom devices" but this file is in the
sc/directory and implements the SC device generator.Apply this diff:
/** @file - * @brief The MQT QDMI device generator for neutral atom devices. + * @brief The MQT QDMI device generator for superconducting devices. */
17-35: Remove unused includes.Static analysis flags several unused headers:
algorithm,cstddef,functional,ranges,tuple, andutility.Apply this diff:
#include "qdmi/sc/Generator.hpp" -#include <algorithm> #include <cassert> #include <cmath> -#include <cstddef> #include <cstdint> #include <fstream> -#include <functional> #include <istream> #include <nlohmann/json.hpp> #include <ostream> -#include <ranges> #include <spdlog/spdlog.h> #include <sstream> #include <stdexcept> #include <string> -#include <tuple> -#include <utility> #include <vector>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
include/mqt-core/qdmi/sc/Device.hpp(1 hunks)src/qdmi/sc/App.cpp(1 hunks)src/qdmi/sc/Generator.cpp(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: marcelwa
Repo: munich-quantum-toolkit/core PR: 1243
File: test/python/qdmi/qiskit/test_qdmi_qiskit_backend.py:0-0
Timestamp: 2025-11-04T14:28:32.371Z
Learning: In the munich-quantum-toolkit/core repository, at least one FoMaC device is always available during testing, so skip logic for missing devices in QDMI Qiskit backend tests is not necessary.
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1287
File: test/qdmi/dd/CMakeLists.txt:9-21
Timestamp: 2025-11-03T23:09:26.881Z
Learning: The CMake functions `generate_device_defs_executable` and `generate_prefixed_qdmi_headers` used in QDMI device test CMakeLists.txt files are provided by the external QDMI library (fetched via FetchContent from https://github.com/Munich-Quantum-Software-Stack/qdmi.git), specifically in the cmake/PrefixHandling.cmake module of the QDMI repository.
📚 Learning: 2025-11-03T23:09:26.881Z
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1287
File: test/qdmi/dd/CMakeLists.txt:9-21
Timestamp: 2025-11-03T23:09:26.881Z
Learning: The CMake functions `generate_device_defs_executable` and `generate_prefixed_qdmi_headers` used in QDMI device test CMakeLists.txt files are provided by the external QDMI library (fetched via FetchContent from https://github.com/Munich-Quantum-Software-Stack/qdmi.git), specifically in the cmake/PrefixHandling.cmake module of the QDMI repository.
Applied to files:
src/qdmi/sc/Generator.cpp
📚 Learning: 2025-10-17T11:09:50.147Z
Learnt from: marcelwa
Repo: munich-quantum-toolkit/core PR: 1243
File: test/python/qdmi/qiskit/test_qdmi_qiskit_site_handling.py:162-173
Timestamp: 2025-10-17T11:09:50.147Z
Learning: For QDMI tests that reference a specific named device via fixture (e.g., "MQT NA Default QDMI Device"), it is acceptable to hard-code device-specific values such as zone counts and indices, as these tests are intentionally coupled to that device's structure and serve as structural invariant checks.
Applied to files:
include/mqt-core/qdmi/sc/Device.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/mqt-core/qdmi/sc/Device.hpp
📚 Learning: 2025-11-04T14:28:32.371Z
Learnt from: marcelwa
Repo: munich-quantum-toolkit/core PR: 1243
File: test/python/qdmi/qiskit/test_qdmi_qiskit_backend.py:0-0
Timestamp: 2025-11-04T14:28:32.371Z
Learning: In the munich-quantum-toolkit/core repository, at least one FoMaC device is always available during testing, so skip logic for missing devices in QDMI Qiskit backend tests is not necessary.
Applied to files:
include/mqt-core/qdmi/sc/Device.hpp
🧬 Code graph analysis (1)
src/qdmi/sc/App.cpp (1)
src/qdmi/sc/Generator.cpp (10)
writeJSONSchema(114-127)writeJSONSchema(114-114)writeJSONSchema(139-150)writeJSONSchema(139-139)readJSON(162-162)readJSON(185-185)writeHeader(207-213)writeHeader(207-207)writeHeader(225-235)writeHeader(225-225)
🪛 GitHub Check: 🇨 Lint / 🚨 Lint
src/qdmi/sc/Generator.cpp
[warning] 33-33: src/qdmi/sc/Generator.cpp:33:1 [misc-include-cleaner]
included header utility is not used directly
[warning] 32-32: src/qdmi/sc/Generator.cpp:32:1 [misc-include-cleaner]
included header tuple is not used directly
[warning] 27-27: src/qdmi/sc/Generator.cpp:27:1 [misc-include-cleaner]
included header ranges is not used directly
[warning] 23-23: src/qdmi/sc/Generator.cpp:23:1 [misc-include-cleaner]
included header functional is not used directly
[warning] 20-20: src/qdmi/sc/Generator.cpp:20:1 [misc-include-cleaner]
included header cstddef is not used directly
[warning] 17-17: src/qdmi/sc/Generator.cpp:17:1 [misc-include-cleaner]
included header algorithm is not used directly
🔇 Additional comments (13)
src/qdmi/sc/App.cpp (5)
216-233: LGTM: Schema argument parsing is robust.The function correctly handles help flags and output options with appropriate error handling for missing values.
302-331: LGTM: Schema command execution is well-structured.The function properly handles argument parsing, help output, and schema generation with appropriate error handling and logging.
346-368: LGTM: Validate command execution is correct.The function correctly validates JSON input from file or stdin, with appropriate error handling.
383-420: LGTM: Generate command execution is well-implemented.The function properly reads device configuration from JSON and generates header output with appropriate error handling.
435-484: LGTM: Main function is well-structured with appropriate error handling.The function correctly builds the argument vector, parses global options, handles help/version flags, and dispatches to the appropriate subcommand executor. The try-catch blocks ensure graceful error handling throughout.
src/qdmi/sc/Generator.cpp (4)
46-48: LGTM: Default array population for schema generation.This helper correctly ensures the device has at least one coupling element for schema generation purposes.
114-150: LGTM: JSON schema writing is well-implemented.Both the stream and file-based versions are correctly implemented with appropriate error handling and logging.
162-194: LGTM: JSON reading is well-implemented.Both the stream and file-based versions correctly parse JSON with appropriate error handling.
207-235: LGTM: Header writing correctly maintains macro order dependency.The function properly writes the header guard and device initialization macros in the correct order, maintaining the dependency between
writeSitesandwriteCouplingMap.include/mqt-core/qdmi/sc/Device.hpp (4)
62-88: LGTM: Device API is well-documented and follows QDMI patterns.The public API correctly implements session management and property queries, with appropriate documentation references to the C API.
94-158: LGTM: Session implementation structure is well-designed.The session implementation correctly tracks lifecycle state and manages job ownership with appropriate QDMI API methods.
237-260: LGTM: Site implementation uses factory pattern correctly.The site implementation correctly encapsulates the ID and provides a factory function for creation, with appropriate property query support.
265-283: LGTM: Operation implementation structure is appropriate.The operation implementation correctly uses a factory pattern and provides property query support with site and parameter arrays.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
include/mqt-core/qdmi/sc/Device.hpp(1 hunks)include/mqt-core/qdmi/sc/Generator.hpp(1 hunks)json/sc/device.json(1 hunks)src/qdmi/sc/Device.cpp(1 hunks)src/qdmi/sc/Generator.cpp(1 hunks)test/fomac/test_fomac.cpp(4 hunks)test/qdmi/test_driver.cpp(4 hunks)
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
Learnt from: marcelwa
Repo: munich-quantum-toolkit/core PR: 1243
File: test/python/qdmi/qiskit/test_qdmi_qiskit_backend.py:0-0
Timestamp: 2025-11-04T14:28:32.371Z
Learning: In the munich-quantum-toolkit/core repository, at least one FoMaC device is always available during testing, so skip logic for missing devices in QDMI Qiskit backend tests is not necessary.
Learnt from: marcelwa
Repo: munich-quantum-toolkit/core PR: 1243
File: test/python/qdmi/qiskit/test_qdmi_qiskit_site_handling.py:162-173
Timestamp: 2025-10-17T11:09:50.147Z
Learning: For QDMI tests that reference a specific named device via fixture (e.g., "MQT NA Default QDMI Device"), it is acceptable to hard-code device-specific values such as zone counts and indices, as these tests are intentionally coupled to that device's structure and serve as structural invariant checks.
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1287
File: test/qdmi/dd/CMakeLists.txt:9-21
Timestamp: 2025-11-03T23:09:26.881Z
Learning: The CMake functions `generate_device_defs_executable` and `generate_prefixed_qdmi_headers` used in QDMI device test CMakeLists.txt files are provided by the external QDMI library (fetched via FetchContent from https://github.com/Munich-Quantum-Software-Stack/qdmi.git), specifically in the cmake/PrefixHandling.cmake module of the QDMI repository.
📚 Learning: 2025-11-26T10:29:36.435Z
Learnt from: marcelwa
Repo: munich-quantum-toolkit/core PR: 1243
File: test/python/fomac/test_fomac.py:54-66
Timestamp: 2025-11-26T10:29:36.435Z
Learning: In test/python/fomac/test_fomac.py, the `ddsim_device` fixture returns `Device | None` even though it only returns a `Device` or calls `pytest.skip()`. The `| None` annotation is required for the type checker to pass, as type checkers may not recognize `pytest.skip()` as a function that never returns.
Applied to files:
test/fomac/test_fomac.cpp
📚 Learning: 2025-11-04T14:28:32.371Z
Learnt from: marcelwa
Repo: munich-quantum-toolkit/core PR: 1243
File: test/python/qdmi/qiskit/test_qdmi_qiskit_backend.py:0-0
Timestamp: 2025-11-04T14:28:32.371Z
Learning: In the munich-quantum-toolkit/core repository, at least one FoMaC device is always available during testing, so skip logic for missing devices in QDMI Qiskit backend tests is not necessary.
Applied to files:
test/fomac/test_fomac.cpptest/qdmi/test_driver.cppjson/sc/device.jsonsrc/qdmi/sc/Device.cppinclude/mqt-core/qdmi/sc/Device.hpp
📚 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:
test/fomac/test_fomac.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:
test/fomac/test_fomac.cpp
📚 Learning: 2025-10-17T11:09:50.147Z
Learnt from: marcelwa
Repo: munich-quantum-toolkit/core PR: 1243
File: test/python/qdmi/qiskit/test_qdmi_qiskit_site_handling.py:162-173
Timestamp: 2025-10-17T11:09:50.147Z
Learning: For QDMI tests that reference a specific named device via fixture (e.g., "MQT NA Default QDMI Device"), it is acceptable to hard-code device-specific values such as zone counts and indices, as these tests are intentionally coupled to that device's structure and serve as structural invariant checks.
Applied to files:
test/fomac/test_fomac.cpptest/qdmi/test_driver.cppjson/sc/device.jsonsrc/qdmi/sc/Device.cppinclude/mqt-core/qdmi/sc/Device.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:
test/fomac/test_fomac.cpptest/qdmi/test_driver.cppsrc/qdmi/sc/Device.cppinclude/mqt-core/qdmi/sc/Device.hpp
📚 Learning: 2025-11-03T23:09:26.881Z
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1287
File: test/qdmi/dd/CMakeLists.txt:9-21
Timestamp: 2025-11-03T23:09:26.881Z
Learning: The CMake functions `generate_device_defs_executable` and `generate_prefixed_qdmi_headers` used in QDMI device test CMakeLists.txt files are provided by the external QDMI library (fetched via FetchContent from https://github.com/Munich-Quantum-Software-Stack/qdmi.git), specifically in the cmake/PrefixHandling.cmake module of the QDMI repository.
Applied to files:
test/qdmi/test_driver.cppsrc/qdmi/sc/Generator.cppsrc/qdmi/sc/Device.cppinclude/mqt-core/qdmi/sc/Generator.hppinclude/mqt-core/qdmi/sc/Device.hpp
📚 Learning: 2025-11-04T19:50:14.489Z
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1287
File: test/qdmi/dd/helpers/test_utils.cpp:103-118
Timestamp: 2025-11-04T19:50:14.489Z
Learning: In the munich-quantum-toolkit/core QDMI device tests, when QDMI APIs return a string result size `ks` that includes a null terminator, the pattern `std::string keys(ks - 1, '\0'); ... device_job_get_results(..., ks, keys.data(), nullptr)` is correct and safe. The string allocates `ks` bytes total (ks-1 content + implicit null), and the API writing `ks` bytes (including trailing null) fits exactly into this space without buffer overrun.
Applied to files:
test/qdmi/test_driver.cppsrc/qdmi/sc/Device.cpp
🧬 Code graph analysis (4)
test/fomac/test_fomac.cpp (2)
include/mqt-core/fomac/FoMaC.hpp (9)
sites(493-495)sites(498-500)sites(503-504)sites(507-509)sites(512-514)sites(517-519)sites(522-524)sites(527-529)sites(549-551)src/na/device/Device.cpp (2)
site(356-356)site(364-364)
src/qdmi/sc/Generator.cpp (2)
include/mqt-core/qdmi/sc/Generator.hpp (6)
device(111-111)device(122-122)os(73-73)path(82-82)path(100-100)is(91-91)include/mqt-core/qdmi/sc/Device.hpp (1)
id(266-267)
src/qdmi/sc/Device.cpp (2)
include/mqt-core/qdmi/sc/Device.hpp (11)
prop(100-101)prop(152-153)prop(213-214)prop(272-273)Device(52-52)Device(59-59)Device(62-62)Device(66-66)session(88-88)session(94-94)id(266-267)src/qdmi/Driver.cpp (8)
sessionAlloc(369-377)sessionAlloc(369-369)sessionFree(379-383)sessionFree(379-379)queryProperty(260-264)queryProperty(260-261)queryDeviceProperty(190-195)queryDeviceProperty(190-192)
include/mqt-core/qdmi/sc/Generator.hpp (1)
src/qdmi/sc/Generator.cpp (2)
nodiscard(179-191)nodiscard(202-211)
⏰ 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 (windows-2022) / 🐍 windows-2022
- GitHub Check: 🐍 Test (ubuntu-24.04) / 🐍 ubuntu-24.04
- GitHub Check: 🐍 Test (macos-15-intel) / 🐍 macos-15-intel
🔇 Additional comments (5)
test/fomac/test_fomac.cpp (1)
17-17: LGTM! Refactor tounique_ptrpattern correctly implemented.The refactor from value members to
std::unique_ptr<Site>andstd::unique_ptr<Operation>is correctly implemented:
#include <memory>added appropriately.SetUp()initializes the pointers viastd::make_uniqueafter asserting non-empty collections, avoiding null dereferences.- All test methods consistently use
->operator.- Hard assertions (
ASSERT_FALSE(sites.empty())/ASSERT_FALSE(operations.empty())) align with the maintainer's confirmed expectation that all FoMaC devices must have sites and operations.Based on past review comments, the null-dereference concern was addressed and operations were added to the SC device per burgholzer's request.
Also applies to: 37-54, 274-372
include/mqt-core/qdmi/sc/Generator.hpp (1)
1-123: LGTM!The Generator header is well-structured with:
- Complete includes (including for std::pair)
- Clear Device schema with proper JSON serialization macros
- Comprehensive API for JSON schema generation, reading, and header writing
- Good documentation with doxygen comments
All past review concerns have been addressed.
src/qdmi/sc/Generator.cpp (1)
1-254: LGTM!The Generator implementation is solid:
- Clear helper functions for macro generation
- Proper error handling with exceptions and logging
- Well-documented macro dependencies (INITIALIZE_SITES before INITIALIZE_COUPLINGMAP)
- Correct use of value returns instead of const references
- All past review issues have been addressed
include/mqt-core/qdmi/sc/Device.hpp (1)
1-305: LGTM!The Device header demonstrates good design:
- Thread-safe singleton pattern with proper lifecycle management
- Session status tracking for state validation
- All includes are self-contained
- Out-of-line destructor for proper unique_ptr usage
- Job methods correctly declared as instance methods
- Factory patterns for Site and Operation creation
- Clear separation between Device, Session, Job, Site, and Operation concerns
All previously identified issues have been resolved.
src/qdmi/sc/Device.cpp (1)
1-501: LGTM!The Device implementation is well-structured and correct:
- Thread-safe singleton: Proper use of atomic operations with compare_exchange_strong for initialization
- Lifecycle enforcement: Sessions must be INITIALIZED before creating jobs, preventing misuse
- Argument validation: All C API entry points validate handles and parameters before delegating
- Minimal device semantics: Job operations correctly return NOTSUPPORTED while maintaining API completeness
- Property queries: Well-implemented using macros for consistency (ADD_STRING_PROPERTY, ADD_SINGLE_VALUE_PROPERTY, ADD_LIST_PROPERTY)
- STRNCPY safety: Correctly uses (size-1) to avoid buffer overruns on both Windows and POSIX
The acknowledged lack of concurrency protection for sessions_/jobs_ maps is acceptable for the current single-threaded usage and documented for future work.
All past review concerns have been addressed.
burgholzer
left a comment
burgholzer
left a comment
There was a problem hiding this comment.
Two last points before getting this in.
There was a problem hiding this comment.
I am happy once CI is happy here. Thanks for your work @ystade!
Edit: CI does not really seem happy yet.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
json/sc/device.json (1)
186-189: AlignrgatenumParameterswith Qiskit RGate (2 parameters, not 3).The
"r"operation is configured with"numParameters": 3, but the corresponding QiskitRGateonly takes two parameters (theta,phi). Keeping this at 3 will misrepresent the gate in QDMI metadata and likely confuse or break Qiskit-side tooling that expects the standard 2‑parameter R gate.Unless you intentionally model a different 3‑parameter gate here, please change this to 2 (or rename the operation if it should remain 3‑parameter):
- "operations": [ - { "name": "r", "numParameters": 3, "numQubits": 1 }, - { "name": "cz", "numParameters": 0, "numQubits": 2 } - ] + "operations": [ + { "name": "r", "numParameters": 2, "numQubits": 1 }, + { "name": "cz", "numParameters": 0, "numQubits": 2 } + ]According to the latest Qiskit documentation, how many parameters does qiskit.circuit.library.RGate take, and what are they called?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
include/mqt-core/qdmi/sc/Device.hpp(1 hunks)json/sc/device.json(1 hunks)src/qdmi/sc/Device.cpp(1 hunks)src/qdmi/sc/Generator.cpp(1 hunks)
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: marcelwa
Repo: munich-quantum-toolkit/core PR: 1243
File: test/python/qdmi/qiskit/test_qdmi_qiskit_backend.py:0-0
Timestamp: 2025-11-04T14:28:32.371Z
Learning: In the munich-quantum-toolkit/core repository, at least one FoMaC device is always available during testing, so skip logic for missing devices in QDMI Qiskit backend tests is not necessary.
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1287
File: test/qdmi/dd/CMakeLists.txt:9-21
Timestamp: 2025-11-03T23:09:26.881Z
Learning: The CMake functions `generate_device_defs_executable` and `generate_prefixed_qdmi_headers` used in QDMI device test CMakeLists.txt files are provided by the external QDMI library (fetched via FetchContent from https://github.com/Munich-Quantum-Software-Stack/qdmi.git), specifically in the cmake/PrefixHandling.cmake module of the QDMI repository.
Learnt from: marcelwa
Repo: munich-quantum-toolkit/core PR: 1243
File: test/python/qdmi/qiskit/test_qdmi_qiskit_site_handling.py:162-173
Timestamp: 2025-10-17T11:09:50.147Z
Learning: For QDMI tests that reference a specific named device via fixture (e.g., "MQT NA Default QDMI Device"), it is acceptable to hard-code device-specific values such as zone counts and indices, as these tests are intentionally coupled to that device's structure and serve as structural invariant checks.
📚 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:
json/sc/device.json
📚 Learning: 2025-10-17T11:09:50.147Z
Learnt from: marcelwa
Repo: munich-quantum-toolkit/core PR: 1243
File: test/python/qdmi/qiskit/test_qdmi_qiskit_site_handling.py:162-173
Timestamp: 2025-10-17T11:09:50.147Z
Learning: For QDMI tests that reference a specific named device via fixture (e.g., "MQT NA Default QDMI Device"), it is acceptable to hard-code device-specific values such as zone counts and indices, as these tests are intentionally coupled to that device's structure and serve as structural invariant checks.
Applied to files:
json/sc/device.jsonsrc/qdmi/sc/Device.cppinclude/mqt-core/qdmi/sc/Device.hpp
📚 Learning: 2025-11-04T14:28:32.371Z
Learnt from: marcelwa
Repo: munich-quantum-toolkit/core PR: 1243
File: test/python/qdmi/qiskit/test_qdmi_qiskit_backend.py:0-0
Timestamp: 2025-11-04T14:28:32.371Z
Learning: In the munich-quantum-toolkit/core repository, at least one FoMaC device is always available during testing, so skip logic for missing devices in QDMI Qiskit backend tests is not necessary.
Applied to files:
json/sc/device.jsonsrc/qdmi/sc/Device.cppinclude/mqt-core/qdmi/sc/Device.hpp
📚 Learning: 2025-11-03T23:09:26.881Z
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1287
File: test/qdmi/dd/CMakeLists.txt:9-21
Timestamp: 2025-11-03T23:09:26.881Z
Learning: The CMake functions `generate_device_defs_executable` and `generate_prefixed_qdmi_headers` used in QDMI device test CMakeLists.txt files are provided by the external QDMI library (fetched via FetchContent from https://github.com/Munich-Quantum-Software-Stack/qdmi.git), specifically in the cmake/PrefixHandling.cmake module of the QDMI repository.
Applied to files:
src/qdmi/sc/Generator.cppsrc/qdmi/sc/Device.cppinclude/mqt-core/qdmi/sc/Device.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:
src/qdmi/sc/Device.cppinclude/mqt-core/qdmi/sc/Device.hpp
📚 Learning: 2025-11-04T19:50:14.489Z
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1287
File: test/qdmi/dd/helpers/test_utils.cpp:103-118
Timestamp: 2025-11-04T19:50:14.489Z
Learning: In the munich-quantum-toolkit/core QDMI device tests, when QDMI APIs return a string result size `ks` that includes a null terminator, the pattern `std::string keys(ks - 1, '\0'); ... device_job_get_results(..., ks, keys.data(), nullptr)` is correct and safe. The string allocates `ks` bytes total (ks-1 content + implicit null), and the API writing `ks` bytes (including trailing null) fits exactly into this space without buffer overrun.
Applied to files:
src/qdmi/sc/Device.cpp
📚 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/qdmi/sc/Device.cpp
🧬 Code graph analysis (2)
src/qdmi/sc/Generator.cpp (2)
include/mqt-core/qdmi/sc/Generator.hpp (5)
device(111-111)device(122-122)os(73-73)path(82-82)path(100-100)include/mqt-core/qdmi/sc/Device.hpp (1)
id(267-268)
include/mqt-core/qdmi/sc/Device.hpp (1)
src/qdmi/sc/Device.cpp (3)
Device(101-110)Device(111-114)site(326-326)
🪛 GitHub Check: 🇨 Lint / 🚨 Lint
src/qdmi/sc/Device.cpp
[warning] 366-366: src/qdmi/sc/Device.cpp:366:62 [cppcoreguidelines-use-default-member-init]
member initializer for 'numQubits_' is redundant
[warning] 109-109: src/qdmi/sc/Device.cpp:109:3 [bugprone-use-after-move]
'_couplings' used after it was moved
[warning] 99-99: src/qdmi/sc/Device.cpp:99:6 [misc-include-cleaner]
no header providing "std::atomic" is directly included
⏰ 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). (4)
- GitHub Check: 🐍 Test (windows-2022) / 🐍 windows-2022
- GitHub Check: 🐍 Test (macos-15-intel) / 🐍 macos-15-intel
- GitHub Check: 🐍 Test (ubuntu-24.04) / 🐍 ubuntu-24.04
- GitHub Check: 🇨 Test 🍎 (macos-14, clang, Debug) / 🍎 macos-14 clang Debug
🔇 Additional comments (1)
src/qdmi/sc/Device.cpp (1)
525-525: GuardMQT_SC_QDMI_device_job_freeagainst null job handles.Unlike the other C wrappers,
MQT_SC_QDMI_device_job_freedereferencesjobunconditionally. A bad caller passingnullptrwill segfault here, whilesessionFreeand all job accessors defensively validate their handles.Given the function returns
void, treating a null job as a no-op is consistent with howsessionFreebehaves:-void MQT_SC_QDMI_device_job_free(MQT_SC_QDMI_Device_Job job) { job->free(); } +void MQT_SC_QDMI_device_job_free(MQT_SC_QDMI_Device_Job job) { + if (job == nullptr) { + return; + } + job->free(); +}This removes a potential crash point without affecting valid callers.
Signed-off-by: burgholzer <burgholzer@me.com>
Signed-off-by: burgholzer <burgholzer@me.com>
Signed-off-by: burgholzer <burgholzer@me.com>
Signed-off-by: burgholzer <burgholzer@me.com>
Signed-off-by: burgholzer <burgholzer@me.com>
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
src/qdmi/sc/Device.cpp (2)
364-369: Optional: redundantnumQubits_(1)initialization.The single-qubit constructor explicitly initializes
numQubits_(1)even though the member has a default initializer of 1 in the class definition, triggering acppcoreguidelines-use-default-member-initlint warning. You can dropnumQubits_(1)from the initializer list here to rely on the default, though keeping it may provide symmetry with the two-qubit constructor (line 374) wherenumQubits_(2)is necessary.If you prefer to silence the lint:
MQT_SC_QDMI_Operation_impl_d::MQT_SC_QDMI_Operation_impl_d( std::string name, const size_t numParameters, const std::vector<MQT_SC_QDMI_Site>& sites) - : name_(std::move(name)), numParameters_(numParameters), numQubits_(1), + : name_(std::move(name)), numParameters_(numParameters), supportedSites_(sites) {
526-526: Add null check toMQT_SC_QDMI_device_job_freefor consistency and safety.All other C API entry points validate their handle arguments before dereferencing (e.g., lines 499, 531, 541, 548, etc.), but
device_job_freedirectly callsjob->free()without checking ifjobis null. While well-behaved callers should never pass null, adding a guard improves robustness and consistency with the rest of the API surface.Apply this diff to add the null check:
-void MQT_SC_QDMI_device_job_free(MQT_SC_QDMI_Device_Job job) { job->free(); } +void MQT_SC_QDMI_device_job_free(MQT_SC_QDMI_Device_Job job) { + if (job != nullptr) { + job->free(); + } +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
src/qdmi/sc/Device.cpp(1 hunks)src/qdmi/sc/Generator.cpp(1 hunks)
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
Learnt from: marcelwa
Repo: munich-quantum-toolkit/core PR: 1243
File: test/python/qdmi/qiskit/test_qdmi_qiskit_backend.py:0-0
Timestamp: 2025-11-04T14:28:32.371Z
Learning: In the munich-quantum-toolkit/core repository, at least one FoMaC device is always available during testing, so skip logic for missing devices in QDMI Qiskit backend tests is not necessary.
📚 Learning: 2025-11-03T23:09:26.881Z
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1287
File: test/qdmi/dd/CMakeLists.txt:9-21
Timestamp: 2025-11-03T23:09:26.881Z
Learning: The CMake functions `generate_device_defs_executable` and `generate_prefixed_qdmi_headers` used in QDMI device test CMakeLists.txt files are provided by the external QDMI library (fetched via FetchContent from https://github.com/Munich-Quantum-Software-Stack/qdmi.git), specifically in the cmake/PrefixHandling.cmake module of the QDMI repository.
Applied to files:
src/qdmi/sc/Generator.cppsrc/qdmi/sc/Device.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:
src/qdmi/sc/Generator.cppsrc/qdmi/sc/Device.cpp
📚 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/qdmi/sc/Generator.cpp
📚 Learning: 2025-10-17T11:09:50.147Z
Learnt from: marcelwa
Repo: munich-quantum-toolkit/core PR: 1243
File: test/python/qdmi/qiskit/test_qdmi_qiskit_site_handling.py:162-173
Timestamp: 2025-10-17T11:09:50.147Z
Learning: For QDMI tests that reference a specific named device via fixture (e.g., "MQT NA Default QDMI Device"), it is acceptable to hard-code device-specific values such as zone counts and indices, as these tests are intentionally coupled to that device's structure and serve as structural invariant checks.
Applied to files:
src/qdmi/sc/Device.cpp
📚 Learning: 2025-11-04T19:50:14.489Z
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1287
File: test/qdmi/dd/helpers/test_utils.cpp:103-118
Timestamp: 2025-11-04T19:50:14.489Z
Learning: In the munich-quantum-toolkit/core QDMI device tests, when QDMI APIs return a string result size `ks` that includes a null terminator, the pattern `std::string keys(ks - 1, '\0'); ... device_job_get_results(..., ks, keys.data(), nullptr)` is correct and safe. The string allocates `ks` bytes total (ks-1 content + implicit null), and the API writing `ks` bytes (including trailing null) fits exactly into this space without buffer overrun.
Applied to files:
src/qdmi/sc/Device.cpp
📚 Learning: 2025-11-04T14:28:32.371Z
Learnt from: marcelwa
Repo: munich-quantum-toolkit/core PR: 1243
File: test/python/qdmi/qiskit/test_qdmi_qiskit_backend.py:0-0
Timestamp: 2025-11-04T14:28:32.371Z
Learning: In the munich-quantum-toolkit/core repository, at least one FoMaC device is always available during testing, so skip logic for missing devices in QDMI Qiskit backend tests is not necessary.
Applied to files:
src/qdmi/sc/Device.cpp
📚 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/qdmi/sc/Device.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/qdmi/sc/Device.cpp
🧬 Code graph analysis (1)
src/qdmi/sc/Generator.cpp (3)
include/mqt-core/qdmi/sc/Generator.hpp (6)
device(111-111)device(122-122)os(73-73)path(82-82)path(100-100)is(91-91)include/mqt-core/qdmi/sc/Device.hpp (1)
id(267-268)test/qdmi/sc/test_device.cpp (1)
nodiscard(23-41)
⏰ 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 (ubuntu-24.04) / 🐍 ubuntu-24.04
- GitHub Check: 🐍 Test (ubuntu-24.04-arm) / 🐍 ubuntu-24.04-arm
- GitHub Check: 🐍 Test (windows-2022) / 🐍 windows-2022
🔇 Additional comments (2)
src/qdmi/sc/Device.cpp (2)
99-198: LGTM: Device singleton and property query implementation.The Device singleton lifecycle (lines 116–139) uses proper atomic operations (
compare_exchange_strong,exchange) for thread-safe initialization and finalization. Session and job management viastd::unique_ptrin maps ensures automatic cleanup, and the explicitsessions_.clear()in the destructor (line 114) prevents use-after-free edge cases. The macro-based property query pattern (lines 158–198) centralizes validation and reduces repetition across device, site, and operation queries.
392-482: LGTM: Operation property query logic handles single- and two-qubit cases correctly.The
queryPropertyimplementation (lines 392–482) usesstd::visitto dispatch on thesupportedSites_variant, validating single-qubit sites viabinary_search(line 410) and two-qubit pairs by normalizing and searching (lines 421–432). The reinterpret_cast for two-qubit site flattening (lines 469–472) is safe given thestatic_assertchecks (lines 461–465) confirming standard layout and expected size.
Signed-off-by: burgholzer <burgholzer@me.com>
Signed-off-by: burgholzer <burgholzer@me.com>
…sites into account Signed-off-by: burgholzer <burgholzer@me.com>
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 (2)
src/qdmi/sc/Generator.cpp(1 hunks)test/fomac/test_fomac.cpp(2 hunks)
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
Learnt from: marcelwa
Repo: munich-quantum-toolkit/core PR: 1243
File: test/python/qdmi/qiskit/test_qdmi_qiskit_site_handling.py:162-173
Timestamp: 2025-10-17T11:09:50.147Z
Learning: For QDMI tests that reference a specific named device via fixture (e.g., "MQT NA Default QDMI Device"), it is acceptable to hard-code device-specific values such as zone counts and indices, as these tests are intentionally coupled to that device's structure and serve as structural invariant checks.
📚 Learning: 2025-11-26T10:29:36.435Z
Learnt from: marcelwa
Repo: munich-quantum-toolkit/core PR: 1243
File: test/python/fomac/test_fomac.py:54-66
Timestamp: 2025-11-26T10:29:36.435Z
Learning: In test/python/fomac/test_fomac.py, the `ddsim_device` fixture returns `Device | None` even though it only returns a `Device` or calls `pytest.skip()`. The `| None` annotation is required for the type checker to pass, as type checkers may not recognize `pytest.skip()` as a function that never returns.
Applied to files:
test/fomac/test_fomac.cpp
📚 Learning: 2025-11-04T14:28:32.371Z
Learnt from: marcelwa
Repo: munich-quantum-toolkit/core PR: 1243
File: test/python/qdmi/qiskit/test_qdmi_qiskit_backend.py:0-0
Timestamp: 2025-11-04T14:28:32.371Z
Learning: In the munich-quantum-toolkit/core repository, at least one FoMaC device is always available during testing, so skip logic for missing devices in QDMI Qiskit backend tests is not necessary.
Applied to files:
test/fomac/test_fomac.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:
test/fomac/test_fomac.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:
test/fomac/test_fomac.cpp
📚 Learning: 2025-10-17T11:09:50.147Z
Learnt from: marcelwa
Repo: munich-quantum-toolkit/core PR: 1243
File: test/python/qdmi/qiskit/test_qdmi_qiskit_site_handling.py:162-173
Timestamp: 2025-10-17T11:09:50.147Z
Learning: For QDMI tests that reference a specific named device via fixture (e.g., "MQT NA Default QDMI Device"), it is acceptable to hard-code device-specific values such as zone counts and indices, as these tests are intentionally coupled to that device's structure and serve as structural invariant checks.
Applied to files:
test/fomac/test_fomac.cpp
📚 Learning: 2025-11-03T23:09:26.881Z
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1287
File: test/qdmi/dd/CMakeLists.txt:9-21
Timestamp: 2025-11-03T23:09:26.881Z
Learning: The CMake functions `generate_device_defs_executable` and `generate_prefixed_qdmi_headers` used in QDMI device test CMakeLists.txt files are provided by the external QDMI library (fetched via FetchContent from https://github.com/Munich-Quantum-Software-Stack/qdmi.git), specifically in the cmake/PrefixHandling.cmake module of the QDMI repository.
Applied to files:
src/qdmi/sc/Generator.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:
src/qdmi/sc/Generator.cpp
📚 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/qdmi/sc/Generator.cpp
🧬 Code graph analysis (1)
src/qdmi/sc/Generator.cpp (2)
include/mqt-core/qdmi/sc/Generator.hpp (6)
device(111-111)device(122-122)os(73-73)path(82-82)path(100-100)is(91-91)include/mqt-core/qdmi/sc/Device.hpp (1)
id(267-268)
⏰ 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 (12)
src/qdmi/sc/Generator.cpp (9)
1-27: LGTM!File header and includes are clean. The file description correctly refers to superconducting devices (past issue resolved), and the includes are minimal and necessary.
28-62: LGTM!The helper functions
populateArrayFields,writeName, andwriteQubitsNumare straightforward and correctly generate initialization macros. ThepopulateArrayFieldsfunction ensures the JSON schema template has example entries.
76-98: LGTM with a dependency note.The
writeSitesfunction correctly generates the site and coupling initialization logic. The_singleQubitSitesand_couplingsvectors created here are properly used by subsequent macros (see documentation at lines 138-140).
100-130: LGTM!The
writeOperationsfunction correctly generates operation initialization code for single- and two-qubit operations. The design choice to support only 1- and 2-qubit operations is reasonable for SC devices, with appropriate error handling for unsupported operation counts.
132-146: LGTM!The
writeCouplingMapfunction and its documentation correctly describe the copy semantics and dependency on the_couplingsvariable created byINITIALIZE_SITES. Past documentation issue has been properly resolved.
149-185: LGTM!Both
writeJSONSchemaoverloads are well-implemented with proper error handling and file management. The NOLINTNEXTLINE suppression formisc-include-cleanerat line 157 is appropriate for nlohmann::json's template metaprogramming.
187-229: LGTM!Both
readJSONoverloads are correctly implemented with proper value semantics (line 226 correctly returns by value, past issue resolved) and comprehensive error handling for file I/O and JSON parsing.
256-276: LGTM!The
writeHeaderfile overload is well-implemented with proper error handling and logging.
83-88: Consider usingstd::transformfor C++17 compatibility.The generated code uses
std::ranges::transform(a C++20 feature). For broader compatibility, consider usingstd::transformfrom<algorithm>instead, which works in C++17 and later:-os << ";\\\n std::ranges::transform(var, " - "std::back_inserter(_singleQubitSites), [](const " - "std::unique_ptr<MQT_SC_QDMI_Site_impl_d>& site) { return site.get(); " - "})"; +os << ";\\\n std::transform(var.begin(), var.end(), " + "std::back_inserter(_singleQubitSites), [](const auto& site) { " + "return site.get(); })";This would eliminate the need for
#include <ranges>in the generated header and maintain compatibility with C++17 compilers.⛔ Skipped due to learnings
Learnt from: DRovara Repo: munich-quantum-toolkit/core PR: 1108 File: mlir/lib/Dialect/MQTOpt/Transforms/ReplaceBasisStateControlsWithIfPattern.cpp:219-221 Timestamp: 2025-10-09T13:14:10.178Z Learning: The MQT Core project (munich-quantum-toolkit/core repository) uses the C++20 standard, not C++17. C++20 features such as abbreviated function templates (e.g., `const auto&` parameters) are supported and valid in this codebase.test/fomac/test_fomac.cpp (3)
34-41: Vector-based site/operation storage in test fixtures looks soundSwitching
SiteTestandOperationTestto store fullstd::vector<...>and populating them inSetUp()viadevice.getSites()/getOperations()withASSERT_FALSE(...empty())is correct and removes the previous pointer lifetime issues. It also makes iterating over all sites/operations straightforward and safe.Also applies to: 44-51
349-365: Operation name/qubits/parameters tests correctly generalized to all operationsIterating over
operationsand checkinggetName(),getQubitsNum(), andgetParametersNum()per operation is correct and aligns with the new vector-based fixture. TheEXPECT_NO_THROW(EXPECT_FALSE(operation.getName().empty()));pattern is consistent with existing tests and no longer has the stray semicolon issue mentioned in earlier reviews.
469-491: TheSitePairstest correctly enforces QDMI specification invariants—no action needed.The hard-coded invariants are not arbitrary device-level rules. The
getSitePairs()method is explicitly documented as interpreting sites "according to the QDMI specification" and the implementation enforces that specification-level contract:
getSitePairs()returnsnulloptfor non-2-qubit operations, zoned operations, or operations with invalid site data- For valid 2-qubit operations, it pairs consecutive sites in the returned list
These are QDMI specification requirements, not device-specific assumptions. Future devices cannot deviate from these invariants without violating the QDMI specification itself. The test correctly verifies specification compliance across all devices.
Signed-off-by: burgholzer <burgholzer@me.com>
Signed-off-by: burgholzer <burgholzer@me.com>
Description
This PR adds a—for the moment—minimalistic SC QDMI device that only features a coupling map. The changes extend the QDMI device framework by relocating the Device class to the qdmi::na namespace, adding a new superconducting (SC) device backend with session and job management APIs, introducing JSON-based device configuration and code generation utilities, and providing a CLI tool and comprehensive test coverage for the SC device.
Checklist: