-
Notifications
You must be signed in to change notification settings - Fork 68
Dem from stim text can use error decompositions #615
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -71,6 +71,11 @@ struct detector_error_model { | |
|
|
||
| /// Parse Stim DEM text into detector/observable flip matrices and error rates. | ||
| /// DEM-native decoders should consume raw DEM text instead. | ||
| detector_error_model dem_from_stim_text(const std::string &dem_text); | ||
| /// If @p decompose_errors is true, error mechanisms that carry an explicit | ||
| /// graphlike decomposition (components separated by '^' in the DEM text) are | ||
| /// expanded into one column per component; otherwise the '^' separators are | ||
| /// ignored and each error instruction produces a single column. | ||
| detector_error_model dem_from_stim_text(const std::string &dem_text, | ||
| bool decompose_errors = false); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I want a refund on my high hopes due this name |
||
|
|
||
| } // namespace cudaq::qec | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -20,7 +20,8 @@ | |||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| namespace cudaq::qec { | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| detector_error_model dem_from_stim_text(const std::string &dem_text) { | ||||||||||||||||||||||||||||||
| detector_error_model dem_from_stim_text(const std::string &dem_text, | ||||||||||||||||||||||||||||||
| bool decompose_errors) { | ||||||||||||||||||||||||||||||
| auto dem = [&dem_text]() { | ||||||||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||||||||
| return stim::DetectorErrorModel(dem_text); | ||||||||||||||||||||||||||||||
|
|
@@ -50,11 +51,10 @@ detector_error_model dem_from_stim_text(const std::string &dem_text) { | |||||||||||||||||||||||||||||
| std::to_string(prob) + | ||||||||||||||||||||||||||||||
| " out of range [0, 1] at instruction index " + | ||||||||||||||||||||||||||||||
| std::to_string(instruction_index)); | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| std::vector<std::size_t> dets; | ||||||||||||||||||||||||||||||
| std::vector<std::size_t> obs; | ||||||||||||||||||||||||||||||
| for (const auto &target : inst.target_data) { | ||||||||||||||||||||||||||||||
| if (target.is_separator()) | ||||||||||||||||||||||||||||||
| continue; | ||||||||||||||||||||||||||||||
| auto push_target = [&](const stim::DemTarget &target) { | ||||||||||||||||||||||||||||||
| if (target.is_relative_detector_id()) { | ||||||||||||||||||||||||||||||
| dets.push_back(static_cast<std::size_t>(target.val())); | ||||||||||||||||||||||||||||||
| } else if (target.is_observable_id()) { | ||||||||||||||||||||||||||||||
|
|
@@ -66,10 +66,38 @@ detector_error_model dem_from_stim_text(const std::string &dem_text) { | |||||||||||||||||||||||||||||
| ") contains an unsupported target kind; only D* (detector) and " | ||||||||||||||||||||||||||||||
| "L* (observable) targets are supported by the fallback parser"); | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| if (decompose_errors) { | ||||||||||||||||||||||||||||||
| // Each segment delimited by '^' in the DEM text becomes its own column. | ||||||||||||||||||||||||||||||
| auto flush = [&]() { | ||||||||||||||||||||||||||||||
| if (!dets.empty() || !obs.empty()) { | ||||||||||||||||||||||||||||||
| detector_hits.push_back(dets); | ||||||||||||||||||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This stores the raw component before duplicate detector targets are XOR-cancelled. For example, Can we canonicalize/XOR-cancel each component before appending, then reject decomposed components with 0 or more than 2 detector endpoints instead of emitting them?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree that it probably makes the most sense to remove all-zero columns, but it looks like there is an existing test case that expects it as an output: cudaqx/libs/qec/unittests/test_decoders.cpp Lines 806 to 819 in 4bb93c6
Does it make sense for these all-zero columns to be removed only when decompose_errors=true, or to change this test? |
||||||||||||||||||||||||||||||
| observable_hits.push_back(obs); | ||||||||||||||||||||||||||||||
| rates.push_back(prob); | ||||||||||||||||||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This changes the probability semantics for decomposed Stim errors. In Stim, Can we avoid exposing decomposed columns as independent error mechanisms, or preserve the correlation explicitly, e.g. with shared
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree with Vedika's concern that this changes the semantics of the error rate vec. I do think that we need to have both meanings. Since |
||||||||||||||||||||||||||||||
| dets.clear(); | ||||||||||||||||||||||||||||||
| obs.clear(); | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||
| for (const auto &target : inst.target_data) { | ||||||||||||||||||||||||||||||
| if (target.is_separator()) { | ||||||||||||||||||||||||||||||
| flush(); | ||||||||||||||||||||||||||||||
| } else { | ||||||||||||||||||||||||||||||
| push_target(target); | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
| flush(); | ||||||||||||||||||||||||||||||
| } else { | ||||||||||||||||||||||||||||||
| // Ignore '^' separators; all targets become a single column. | ||||||||||||||||||||||||||||||
| for (const auto &target : inst.target_data) { | ||||||||||||||||||||||||||||||
| if (target.is_separator()) | ||||||||||||||||||||||||||||||
| continue; | ||||||||||||||||||||||||||||||
| push_target(target); | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
| detector_hits.push_back(std::move(dets)); | ||||||||||||||||||||||||||||||
| observable_hits.push_back(std::move(obs)); | ||||||||||||||||||||||||||||||
| rates.push_back(prob); | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
| detector_hits.push_back(std::move(dets)); | ||||||||||||||||||||||||||||||
| observable_hits.push_back(std::move(obs)); | ||||||||||||||||||||||||||||||
| rates.push_back(prob); | ||||||||||||||||||||||||||||||
| ++instruction_index; | ||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changing the only declaration from
dem_from_stim_text(const std::string&)todem_from_stim_text(const std::string&, bool)preserves source compatibility via the default argument, but removes the old exported C++ symbol. Any already-built downstream code/plugin linked against the one-argument symbol would fail to load.Can we keep the one-argument overload and add a separate two-argument overload? The one-argument overload can simply delegate to
dem_from_stim_text(dem_text, false).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that this is somewhat of a breaking change. However since we don't have a hard downstream dependency (we typically expect downstream to rebuild against the latest main/release), I would prefer that we don't add this overload just to preserve an old mangled symbol.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense. If we expect downstream consumers to rebuild against the latest main/release, I’m fine not preserving the old one-arg mangled symbol here. Thanks for clarifying.