Conversation
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…uting/tsim into rafaelha/underflow_bug
…faelha/underflow_bug
- Added overloads to `_sample_batches` method to handle reference sample computation. - Implemented logic to conditionally XOR detector and observable results with noiseless reference samples. - Introduced warnings for cross-batch reference sample discrepancies. - Updated unit tests to validate new reference sample behavior and ensure existing functionality remains unchanged.
- Updated README and demo notebook to clarify the behavior of TSIM regarding actual detector and observable samples. - Modified the sampler implementation to introduce `use_detector_reference_sample` and `use_observable_reference_sample` parameters, allowing users to compute noiseless reference samples. - Adjusted unit tests to validate the new sampling behavior and ensure correct functionality with the updated parameters.
…faelha/reference_sample
Contributor
|
Contributor
☂️ Python Coverage
Overall Coverage
New FilesNo new covered files... Modified Files
|
Contributor
There was a problem hiding this comment.
Pull request overview
Adds optional “reference sample” behavior to TSIM’s detector sampler so users can XOR sampled detector/observable outputs against a noiseless baseline (matching Stim’s “detection flips” convention), and introduces a runtime check to detect probability-normalization underflow issues during autoregressive sampling.
Changes:
- Add
use_detector_reference_sample/use_observable_reference_sampleoptions toCompiledDetectorSampler.sample, implemented by sampling an extra noiseless row and XORing selected outputs. - Add a marginal-normalization deviation check in component sampling and raise an error when deviation exceeds a threshold.
- Update docs/examples and add unit/integration tests covering the new behavior and the underflow guard.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
src/tsim/sampler.py |
Implements reference-sample XOR options and adds normalization-deviation detection during sampling. |
test/unit/test_sampler.py |
Adds unit tests for reference-sample XOR behavior (including bit-packed output and default behavior). |
test/integration/test_sampler.py |
Adds an integration test ensuring normalization deviation triggers an error; fixes a debug plotting argument. |
README.md |
Documents the new reference-sample flags to match Stim behavior. |
docs/demos/from_stim_to_tsim.ipynb |
Updates demo text/code to show default vs reference-sample behavior. |
src/tsim/core/exact_scalar.py |
Minor TODO wording change. |
Comments suppressed due to low confidence (1)
src/tsim/sampler.py:141
max_norm_deviationis a JAX scalar (jax.Array) returned fromsample_component. Using it directly in a Pythonifand formatting it with:.1eis likely to fail (JAX arrays generally can't be used as booleans/format specifiers without converting to a host scalar). Convert it explicitly (e.g.,dev = float(max_norm_deviation)/.item()) before comparing/formatting, and consider treating NaN as an error (NaN comparisons will bypass the threshold check).
for component in program.components:
samples, key, max_norm_deviation = sample_component(component, f_params, key)
if max_norm_deviation > 1e-5:
raise AssertionError(
"A marginal probability was not normalized correctly "
f"(normalization deviated from 1 by {max_norm_deviation:.1e}). "
"This is likely the result of an underflow error. Please report this "
"as a bug at https://github.com/QuEraComputing/tsim/issues/new."
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
… samples - Adjusted the calculation of total shots and batch sizes when `compute_reference` is enabled. - Updated the logic to ensure correct batch sizing based on available leeway. - Added unit tests to validate the new batch size behavior with and without reference samples.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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 #30
This allows users to draw a noiseless reference sample (to match Stim's behavior).
This is disabled by default, since reference samples can shift the probability distribution in the non-Clifford case.