dem_from_kernel to return DEMResult - #5038
Conversation
Signed-off-by: Andres Paz <andresp@nvidia.com>
Signed-off-by: Andres Paz <andresp@nvidia.com>
CI Summary (
|
| Job | Result | Link |
|---|---|---|
build_and_test |
❌ failure | view |
Top-level jobs (13)
| Job | Result |
|---|---|
binaries |
⏩ skipped |
build_and_test |
❌ failure |
config_devdeps |
✅ success |
config_source_build |
⏩ skipped |
config_wheeldeps |
✅ success |
devdeps |
✅ success |
docker_image |
⏩ skipped |
gen_code_coverage |
⏩ skipped |
metadata |
✅ success |
python_metapackages |
⏩ skipped |
python_wheels |
⏩ skipped |
source_build |
⏩ skipped |
wheeldeps |
✅ success |
⏩ Skipped jobs (7) — intentionally skipped on PR builds; run on merge_group / workflow_dispatch
| Job |
|---|
binaries |
config_source_build |
docker_image |
gen_code_coverage |
python_metapackages |
python_wheels |
source_build |
All sub-jobs (42) — every matrix leg, with links
| Job | Status | Link |
|---|---|---|
| Build and test (amd64, gcc12, openmpi) / Dev environment (Debug) | ✅ success | view |
| Build and test (amd64, gcc12, openmpi) / Dev environment (Python) | ❌ failure | view |
| Build and test (amd64, llvm, openmpi) / Dev environment (Debug) | ✅ success | view |
| Build and test (amd64, llvm, openmpi) / Dev environment (Python) | ❌ failure | view |
| Build and test (arm64, llvm, openmpi) / Dev environment (Debug) | ✅ success | view |
| Build and test (arm64, llvm, openmpi) / Dev environment (Python) | ❌ failure | view |
| CI Summary | ❔ in_progress | view |
| Configure build (devdeps) | ✅ success | view |
| Configure build (source_build) | ⏩ skipped | view |
| Configure build (wheeldeps) | ✅ success | view |
| Create CUDA Quantum installer | ⏩ skipped | view |
| Create Docker images | ⏩ skipped | view |
| Create Python metapackages | ⏩ skipped | view |
| Create Python wheels | ⏩ skipped | view |
| Gen code coverage | ⏩ skipped | view |
| Load dependencies (amd64, gcc12) / Caching | ✅ success | view |
| Load dependencies (amd64, gcc12) / Finalize | ✅ success | view |
| Load dependencies (amd64, gcc12) / Metadata | ✅ success | view |
| Load dependencies (amd64, llvm) / Caching | ✅ success | view |
| Load dependencies (amd64, llvm) / Finalize | ✅ success | view |
| Load dependencies (amd64, llvm) / Metadata | ✅ success | view |
| Load dependencies (arm64, gcc12) / Caching | ✅ success | view |
| Load dependencies (arm64, gcc12) / Finalize | ✅ success | view |
| Load dependencies (arm64, gcc12) / Metadata | ✅ success | view |
| Load dependencies (arm64, llvm) / Caching | ✅ success | view |
| Load dependencies (arm64, llvm) / Finalize | ✅ success | view |
| Load dependencies (arm64, llvm) / Metadata | ✅ success | view |
| Load source build cache | ⏩ skipped | view |
| Load wheel dependencies (amd64, 12.6) / Caching | ✅ success | view |
| Load wheel dependencies (amd64, 12.6) / Finalize | ✅ success | view |
| Load wheel dependencies (amd64, 12.6) / Metadata | ✅ success | view |
| Load wheel dependencies (amd64, 13.0) / Caching | ✅ success | view |
| Load wheel dependencies (amd64, 13.0) / Finalize | ✅ success | view |
| Load wheel dependencies (amd64, 13.0) / Metadata | ✅ success | view |
| Load wheel dependencies (arm64, 12.6) / Caching | ✅ success | view |
| Load wheel dependencies (arm64, 12.6) / Finalize | ✅ success | view |
| Load wheel dependencies (arm64, 12.6) / Metadata | ✅ success | view |
| Load wheel dependencies (arm64, 13.0) / Caching | ✅ success | view |
| Load wheel dependencies (arm64, 13.0) / Finalize | ✅ success | view |
| Load wheel dependencies (arm64, 13.0) / Metadata | ✅ success | view |
| Prepare cache clean-up | ❔ in_progress | view |
| Retrieve PR info | ✅ success | view |
⚠️ Required checks (3/6) — 3 missing — declared in .github/required-checks.yml for push
| Required check | Status | Link |
|---|---|---|
| Build and test (amd64, llvm, openmpi) / Dev environment (Debug) | ✅ success | view |
| Build and test (amd64, llvm, openmpi) / Dev environment (Python) | ❌ failure | view |
| Build and test (arm64, llvm, openmpi) / Dev environment (Debug) | ✅ success | view |
| Build and test (arm64, llvm, openmpi) / Dev environment (Python) | ❌ failure | view |
| Build and test (amd64, gcc12, openmpi) / Dev environment (Debug) | ✅ success | view |
| Build and test (amd64, gcc12, openmpi) / Dev environment (Python) | ❌ failure | view |
|
Seems like we have a test failure ( |
Signed-off-by: Andres Paz <andresp@nvidia.com>
Signed-off-by: Andres Paz <andresp@nvidia.com>
Signed-off-by: Andres Paz <andresp@nvidia.com>
Signed-off-by: Andres Paz <andresp@nvidia.com>
Signed-off-by: Andres Paz <andresp@nvidia.com>
khalatepradnya
left a comment
There was a problem hiding this comment.
These changes are looking good to me. But, more tests need adjustment:
test_Quantinuum_ng_kernel.py:266-268
test_IonQ.py:365-367
I added breaking change tag to reflect the new behavior.
Q: Are we planning to make corresponding C++ API change? Is that tracked anywhere?
|
|
||
| /// @brief The measurement-to-observable sparse matrix. | ||
| M2OSparseMatrix m2o; | ||
|
|
There was a problem hiding this comment.
I would have the same comment here that I had for estimation results. It is a bit tricker here because there is some history and there would be breaking changes since there are no accessors.
But maybe these new members could be moved into an implementation and accessed with accessor methods.
| .def_prop_ro( | ||
| "annotations", | ||
| [](dem_result &self) -> nlohmann::json & { | ||
| auto &j = self.annotations.get(); | ||
| // Default cudaq_json() is null; promote to empty object so the | ||
| // property always returns a mutable dict, matching SampleResult. | ||
| if (j.is_null()) | ||
| j = nlohmann::json::object(); | ||
| return j; | ||
| }, | ||
| "Extensible endpoint metadata dict. Mutate in place: " |
There was a problem hiding this comment.
Let's mark this property as def_prop_rw or drop mutate in place sentence.
| "from_matrices", | ||
| [](const std::string &dem, nanobind::object m2d_csr, | ||
| nanobind::object m2o_csr, std::size_t num_detectors, | ||
| std::size_t num_observables, std::size_t num_measurements, |
There was a problem hiding this comment.
Seems like from_matrices ignores the CSR width, so the default produces a result that blows up later.
| self->num_detectors = num_detectors; | ||
| self->num_observables = num_observables; | ||
| self->num_measurements = num_measurements; | ||
| self->matrices_computed = !m2d.empty() || !m2o.empty(); |
There was a problem hiding this comment.
Should we add an explicit matrices_computed keyword argument (default True) to both __init__ and from_matrices instead of inferring it?
|
|
||
| // Always populate counts — repr and downstream helpers depend on them even | ||
| // when the caller opts out of the full matrices. | ||
| auto flat = recordedCircuit.flattened(); |
There was a problem hiding this comment.
Should we just use recordedCircuit.compute_stats() for the counts, and only call flattened() inside the if branch that needs it?
| f"dem_from_kernel: unknown keyword argument(s) {sorted(unknown)}. " | ||
| f"Valid options: {sorted(_VALID_DEM_OPTION_KEYS)}") | ||
|
|
||
| dem_kwargs.setdefault("return_measurement_matrices", True) |
There was a problem hiding this comment.
Let's keep the default to False and let the user opt in? As now when every caller who wants the DEM text now pays for a second full pass over the flattened circuit plus a sort per detector (here).
| .def_prop_ro( | ||
| "m2d_matrix", | ||
| [](const dem_result &self) -> nanobind::object { | ||
| if (!self.matrices_computed) | ||
| return nanobind::none(); | ||
| auto dem_mod = nanobind::module_::import_("cudaq.runtime.dem"); | ||
| return dem_mod.attr("_make_csr")( | ||
| nanobind::cast(self.m2d.rows), | ||
| nanobind::cast(self.num_measurements)); | ||
| }, | ||
| "scipy CSR matrix (num_detectors x num_measurements), or None when " | ||
| "matrices were not requested.") | ||
| .def_prop_ro( | ||
| "m2o_matrix", | ||
| [](const dem_result &self) -> nanobind::object { | ||
| if (!self.matrices_computed) | ||
| return nanobind::none(); | ||
| auto dem_mod = nanobind::module_::import_("cudaq.runtime.dem"); | ||
| return dem_mod.attr("_make_csr")( | ||
| nanobind::cast(self.m2o.rows), | ||
| nanobind::cast(self.num_measurements)); | ||
| }, | ||
| "scipy CSR matrix (num_observables x num_measurements), or None " | ||
| "when matrices were not requested.") |
There was a problem hiding this comment.
Should we cache the built matrix on the instance? Seems like each property read imports cudaq.runtime.dem module, copies the entire nested std::vector<std::vector<size_t>> into Python lists via that nanobind::cast, then runs two Python level comprehensions in _make_csr. Its costly op.
| /// JSON object <-> dict, array <-> list, string <-> `str`, | ||
| /// integer <-> int, float <-> float, bool <-> bool, null <-> None. | ||
| template <> | ||
| struct type_caster<nlohmann::json> { |
There was a problem hiding this comment.
Should we put these casters into its own header (something like python/utils/JsonCaster.h)? and include it only from py_dem.cpp?
| static nlohmann::json python_to_json(handle src) { | ||
| if (src.is_none()) | ||
| return nullptr; | ||
| if (PyBool_Check(src.ptr())) | ||
| return src.equal(nanobind::bool_(true)); | ||
| if (PyLong_Check(src.ptr())) | ||
| return nanobind::cast<int64_t>(src); | ||
| if (PyFloat_Check(src.ptr())) | ||
| return nanobind::cast<double>(src); | ||
| if (PyUnicode_Check(src.ptr())) | ||
| return nanobind::cast<std::string>(src); | ||
| if (PyDict_Check(src.ptr())) { | ||
| nlohmann::json obj = nlohmann::json::object(); | ||
| for (auto [k, v] : nanobind::borrow<nanobind::dict>(src)) | ||
| obj[nanobind::cast<std::string>(k)] = python_to_json(v); | ||
| return obj; | ||
| } | ||
| if (PyList_Check(src.ptr()) || PyTuple_Check(src.ptr())) { | ||
| nlohmann::json arr = nlohmann::json::array(); | ||
| for (auto item : nanobind::borrow<nanobind::sequence>(src)) | ||
| arr.push_back(python_to_json(item)); | ||
| return arr; | ||
| } | ||
| throw nanobind::type_error("nlohmann::json type caster: unsupported type"); | ||
| } |
There was a problem hiding this comment.
Seems like it recurses through dicts and lists with no depth cap. Let's pass a depth counter and throw nanobind::value_error past some reasonable limit (100 or something).
Closes #5019.
Promotes
cudaq::dem_result— already present in the C++ stack — to the Python boundary as the named typecudaq.DEMResult.dem_from_kernelnow always returns aDEMResult;str(result)still returns the DEM text so existing print calls are unaffected.Changes
result.h— addsnum_detectors,num_observables,num_measurements,matrices_computed, andcudaq_json annotationstocudaq::dem_resultStimCircuitSimulator.cpp— always populates count fields viacompute_stats();computeMeasurementMatricesaccepts a pre-flattened circuit to avoid redundant workpy_dem.cpp— bindsdem_resultasDEMResult;launch_demreturns the object directly instead of a str/tupledem.py— attaches scipy properties (m2d_matrix,m2o_matrix),from_matrices,__str__,__repr__lazily to keep scipy out of the binding layer; flipsreturn_measurement_matricesdefault toTrue__init__.py— exportsDEMResultvia the existing lazy__getattr__mechanismtest_dem_from_kernel.py— rewrites tests under TDD: new tests cover the fullDEMResultcontract; existing tests keep their names, bodies updated for the new return typeMigration
dem_from_kernel(k)→strDEMResult;str(r)unchangeddem_from_kernel(k, return_measurement_matrices=True)→(str, csr, csr)DEMResult; matrices atr.m2d_matrix/r.m2o_matrixstim.DetectorErrorModel(result)stim.DetectorErrorModel(result.dem)