Update stale reversible sampling helper reference#1847
Update stale reversible sampling helper reference#1847vtomole wants to merge 2 commits intoquantumlib:mainfrom
Conversation
…babilities_for_reversible_sampling
There was a problem hiding this comment.
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.
| See `qualtran.linalg.lcu_util.preprocess_probabilities_for_reversible_sampling` | ||
| with `unnormalized_probabilities` for more information. |
There was a problem hiding this comment.
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 n_nonzero_coeff (the sparsity n_nonzero_coeff. Using n_coeff will result in insufficient precision in the prepared state.
| 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. |
The docstring references
preprocess_lcu_coefficients_for_reversible_samplingbut it seems like this method was replaced withpreprocess_probabilities_for_reversible_sampling.