You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
MemStabSimBuilder::noise and DemStabSimBuilder::noise both accept a full NoiseConfig, but neither forwarded it. Each unpacked the caller's config into the four scalar rates and rebuilt a fresh one, discarding every per-gate rate table without reporting anything. A caller who supplied a per-gate calibration got a simulation computed from scalars, with no error and a plausible-looking result.
Changes
mem_stab.rs forwards with_noise_config(self.noise) instead of unpacking into with_noise(p1, p2, p_meas, p_prep).
dem_stab.rs does the same on its non-per_gate_noise branch. This sibling is not named in the issue; it was found by checking whether the same pattern existed elsewhere, and its form is worse: DemSamplerBuilder::with_noise does not merely fail to carry the tables, it explicitly clears them:
while with_noise_config copies them. Fixing only the builder the issue names would have left the DEM path silently wrong.
Both parity tests (mem_stab_tests.rs, dem_stab_tests.rs) were themselves using the lossy unpack to construct the comparison pipeline. Harmless today because those tests use uniform noise with empty tables, but they compared a full-config simulator against a scalars-only pipeline; adding any table would have broken parity for a reason unrelated to parity. Both now use with_noise_config.
Tests
builder_forwards_per_gate_rate_tables in each suite. The MEM test asserts a nonzero CX rate table raises the mechanism total and that the result matches the direct DagFaultAnalyzer + MemBuilder pipeline given the same config -- the parity this file exists to guarantee. The DEM test asserts the rendered detector error model changes.
Asserting only that build succeeds would pass with the tables still dropped, so both pin the effect.
Mutations performed and restored: reverting each builder to the scalar unpack fails its test. MEM reports identical totals (0.006386271615049894 !> 0.006386271615049894); DEM reports an unchanged error model.
These are complementary and neither is sufficient alone. #747 makes MemBuilder validate gate-rate keys; this PR makes the two stab-sim builders actually deliver those keys. Before this change a caller going through MemStabSim or DemStabSim got neither honouring nor rejection, because the tables were cleared before any validation could observe them.
Verification
just rstest debug: 12,839 passed; only the known gpu_density_matrix_two_qubit_roots_preserve_rotation_channels tolerance failure present on dev.
just pytest-ci-core: 9,281 passed, no failures.
just python-ci-lint, pre-commit run --all-files, workspace clippy with -D warnings, cargo fmt --check: all clean.
Added one-qubit coverage after the correctness review noted that both new tests exercised p2_gate_rates only.
The fix forwards both tables, so both needed pinning. builder_forwards_single_qubit_rate_tables in each suite sets p1_gate_rates[H] on a small circuit that actually schedules a one-qubit gate -- the repetition-code fixture has only preparations and measurements, whose rates come from p_prep / p_meas, so a p1 key there would have had no consumer and the test would have passed vacuously.
Mutation: forwarding everything except p1_gate_rates (keeping p2) fails only the two new tests -- 7 passed / 1 failed for MEM, 6 passed / 1 failed for DEM. The pre-existing tests, including the p2 ones added with the fix, pass straight through that regression. That is the gap the review identified, now closed.
Re-verified after the addition: just rstest debug 12,841 passed with only the known gpu_density_matrix_two_qubit_roots_preserve_rotation_channels tolerance failure; just pytest-ci-core 9,281 passed; python lint, workspace clippy with -D warnings, cargo fmt --check, and pre-commit run --all-files all clean.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #771.
MemStabSimBuilder::noiseandDemStabSimBuilder::noiseboth accept a fullNoiseConfig, but neither forwarded it. Each unpacked the caller's config into the four scalar rates and rebuilt a fresh one, discarding every per-gate rate table without reporting anything. A caller who supplied a per-gate calibration got a simulation computed from scalars, with no error and a plausible-looking result.Changes
mem_stab.rsforwardswith_noise_config(self.noise)instead of unpacking intowith_noise(p1, p2, p_meas, p_prep).dem_stab.rsdoes the same on its non-per_gate_noisebranch. This sibling is not named in the issue; it was found by checking whether the same pattern existed elsewhere, and its form is worse:DemSamplerBuilder::with_noisedoes not merely fail to carry the tables, it explicitly clears them:while
with_noise_configcopies them. Fixing only the builder the issue names would have left the DEM path silently wrong.Both parity tests (
mem_stab_tests.rs,dem_stab_tests.rs) were themselves using the lossy unpack to construct the comparison pipeline. Harmless today because those tests use uniform noise with empty tables, but they compared a full-config simulator against a scalars-only pipeline; adding any table would have broken parity for a reason unrelated to parity. Both now usewith_noise_config.Tests
builder_forwards_per_gate_rate_tablesin each suite. The MEM test asserts a nonzeroCXrate table raises the mechanism total and that the result matches the directDagFaultAnalyzer+MemBuilderpipeline given the same config -- the parity this file exists to guarantee. The DEM test asserts the rendered detector error model changes.Asserting only that
buildsucceeds would pass with the tables still dropped, so both pin the effect.Mutations performed and restored: reverting each builder to the scalar unpack fails its test. MEM reports identical totals (
0.006386271615049894 !> 0.006386271615049894); DEM reports an unchanged error model.Relationship to #747
These are complementary and neither is sufficient alone. #747 makes
MemBuildervalidate gate-rate keys; this PR makes the two stab-sim builders actually deliver those keys. Before this change a caller going throughMemStabSimorDemStabSimgot neither honouring nor rejection, because the tables were cleared before any validation could observe them.Verification
just rstest debug: 12,839 passed; only the knowngpu_density_matrix_two_qubit_roots_preserve_rotation_channelstolerance failure present ondev.just pytest-ci-core: 9,281 passed, no failures.just python-ci-lint,pre-commit run --all-files, workspace clippy with-D warnings,cargo fmt --check: all clean.