Skip to content

Update stale reversible sampling helper reference#1847

Open
vtomole wants to merge 2 commits intoquantumlib:mainfrom
vtomole:doc_string_update
Open

Update stale reversible sampling helper reference#1847
vtomole wants to merge 2 commits intoquantumlib:mainfrom
vtomole:doc_string_update

Conversation

@vtomole
Copy link
Copy Markdown
Contributor

@vtomole vtomole commented May 1, 2026

The docstring references preprocess_lcu_coefficients_for_reversible_sampling but it seems like this method was replaced with preprocess_probabilities_for_reversible_sampling.

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request updates a docstring reference in state_preparation_alias_sampling.py to point to the correct utility function. Feedback suggests removing a confusing and inconsistent phrase added to the docstring. Additionally, a bug was identified in the calculation of the precision parameter mu, which incorrectly uses the total number of coefficients instead of the number of non-zero coefficients, potentially leading to insufficient precision in the prepared state.

Comment on lines +463 to +464
See `qualtran.linalg.lcu_util.preprocess_probabilities_for_reversible_sampling`
with `unnormalized_probabilities` for more information.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The update to the function reference is correct, but the added phrase with unnormalized_probabilities is confusing and inconsistent with other docstrings in this file (e.g., line 188). In from_n_coeff, the probabilities are symbolic and not provided as a list.

Additionally, I noticed a bug in the implementation on line 466 (visible in context): mu is calculated using n_coeff (the total number of indices $L$) instead of n_nonzero_coeff (the sparsity $d$). Since alias sampling for a sparse state is performed over the $d$ non-zero coefficients, the precision \nu should be determined by n_nonzero_coeff. Using n_coeff will result in insufficient precision in the prepared state.

Suggested change
See `qualtran.linalg.lcu_util.preprocess_probabilities_for_reversible_sampling`
with `unnormalized_probabilities` for more information.
See qualtran.linalg.lcu_util.preprocess_probabilities_for_reversible_sampling
for more information.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant