fix(noise): honor GeminiNoiseModelABC scaling_factor in squin lowering#786
Conversation
generate_simple_noise_model and generate_logical_noise_model read the raw `*_px` fields off the noise model, which ignore `scaling_factor`. The scaled values only live behind the getter properties, so in the squin/stim path every value of `scaling_factor` produced identical noise. Read the scaled getters instead (mover_pauli_rates, sitter_pauli_rates, local_pauli_rates, global_pauli_rates, cz_unpaired_pauli_rates, and two_qubit_pauli for the correlated CZ rates). The paired-rate lookup now defaults missing keys to 0.0 so a rate that scales to zero can't raise a KeyError. Loss probabilities are left raw on purpose: upstream scaling_factor only scales Pauli rates, not loss (per-category loss scaling is tracked in QuEraComputing/bloqade-circuit#836). Add test_noise_scaling.py, which walks the generated squin kernel IR and asserts the baked-in constants reflect scaling_factor for all noise kernels, that loss stays raw, that scaling_factor=0 zeroes rates without KeyError, and that distinct scales now produce distinct constants (the original bug). Fixes #785 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Pull request overview
Fixes a squin/stim-lowering bug where GeminiNoiseModelABC.scaling_factor was effectively ignored by generate_simple_noise_model / generate_logical_noise_model, by switching from raw *_px fields to the scaled getter APIs for Pauli noise rates.
Changes:
- Read single-qubit Pauli rates via
*_pauli_ratesgetters soscaling_factoris honored in generated squin kernels. - Read correlated CZ Pauli probabilities from
two_qubit_pauli.error_probabilitiesand default missing keys to0.0to avoidKeyErroratscaling_factor=0. - Add a new test suite that inspects squin kernel IR constants to regression-test scaling behavior and ensure loss probabilities remain unscaled.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
python/bloqade/lanes/noise_model.py |
Switches noise parameter sourcing from raw fields to scaled getter APIs during squin kernel construction. |
python/tests/test_noise_scaling.py |
Adds regression tests that walk squin kernel IR constants to verify scaling behavior and loss non-scaling. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
☂️ Code Coverage
Overall Coverage
New FilesNo new covered files... Modified Files
|
Use isinstance(stmt.value, PyAttr) / IList to access .data instead of relying on Data[T], which newer pyright rejects (reportAttributeAccessIssue). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
unwrap() is the idiomatic kirin API for extracting the underlying value from a Data attribute and is typed on the base class, so it needs no PyAttr/IList narrowing and satisfies pyright. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Rename the second cz_unpaired_loss_prob assignment to cz_paired_loss_prob so the paired CZ gate loss no longer shadows the unpaired loss variable. - Derive test expectations from the scaled getter APIs (*_pauli_rates, two_qubit_pauli.error_probabilities) instead of re-deriving raw * scale, so the tests validate that lanes honors the getters and stay valid if upstream changes how scaling is applied. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
jasonhan3
left a comment
There was a problem hiding this comment.
In general, for local_loss_prob, global_loss_prob, cz_gate_loss_prob, cz_unpaired_loss_prob, move_loss_prob, sit_loss_prob, I don't know why you don't call the corresponding getters defined in MoveNoiseModelABC ? It's mentioned in the PR description that you don't but it's unclear to me why, as the issue here: QuEraComputing/bloqade-circuit#836 has a loss_error_scale parameter.
| ) | ||
|
|
||
| cz_unpaired_loss_prob = noise_model.cz_gate_loss_prob | ||
| cz_paired_loss_prob = noise_model.cz_gate_loss_prob |
There was a problem hiding this comment.
shouldn't this be using cz_paired_errors on MoveNoiseModelABC if we want to use the properties instead of the direct field?
There was a problem hiding this comment.
That doesn't include the scale the API's are all messed up upstream
There was a problem hiding this comment.
Ah ok sounds good! So we'll probably update the downstream API's when the upstream ones are updated?
I guess, do we anticipate that the @Property API's here: https://github.com/QuEraComputing/bloqade-circuit/blob/06116fd450aaeefebb4769bc42290f60ece02671/src/bloqade/qasm2/dialects/noise/model.py would have the scale factor? If so we can proactively change it; if not then we can just leave it alone until upstream API's are changed
| if loss: | ||
| groups = ilist.map(pair_qubit, ilist.range(len(controls))) | ||
| squin.broadcast.correlated_qubit_loss(cz_unpaired_loss_prob, groups) | ||
| squin.broadcast.correlated_qubit_loss(cz_paired_loss_prob, groups) |
There was a problem hiding this comment.
same comment; shouldn't this be using cz_paired_errors on MoveNoiseModelABC if we want to use the properties instead of the direct field?
|
Note on scope: this PR only scales the Pauli rates, not the loss probabilities. That's a deliberate limitation of the current upstream API — Per-category loss scaling ( |
Summary
Fixes #785.
generate_simple_noise_model/generate_logical_noise_modelread the raw*_pxfields off theGeminiNoiseModelABC(e.g.noise_model.mover_px), which ignore the model'sscaling_factor. The scaled values only live behind the getter properties (mover_pauli_rates,two_qubit_pauli, …). As a result, in the squin/stim path every value ofscaling_factorproduced identical noise — it worked at the cirq level but was silently a no-op downstream (see the discussion in QuEraComputing/bloqade-circuit#836).Changes
mover_pauli_rates,sitter_pauli_rates,local_pauli_rates,global_pauli_rates,cz_unpaired_pauli_rates.two_qubit_pauli.error_probabilities(scaled), with.get(k, 0.0)so a rate that scales to zero (and gets dropped from the dict) no longer risks aKeyError.scaling_factoronly scales Pauli rates, not loss. Per-category loss scaling is the genuinely-new axis tracked upstream in bloqade-circuit#836. Using the getters here also future-proofs lanes against that upstream work.python/tests/test_noise_scaling.pywalks the generated squin kernel IR and asserts the baked-in constants reflectscaling_factoracross all noise kernels, that loss stays raw, thatscaling_factor=0zeroes rates withoutKeyError, and (regression) that distinct scales now produce distinct constants.Verification
test_noise_scaling.py+test_noise_init_generator.py: 37 passedrewrite/move2squin/test_noise.py+test_device.py: 27 passed, 3 skipped🤖 Generated with Claude Code