Skip to content

Alias sampling and QROM state preparation - #621

Open
Yingrong Chen (YingrongChen) wants to merge 12 commits into
mainfrom
yingrongchen-alias-sampling-qrom-state-prep
Open

Alias sampling and QROM state preparation#621
Yingrong Chen (YingrongChen) wants to merge 12 commits into
mainfrom
yingrongchen-alias-sampling-qrom-state-prep

Conversation

@YingrongChen

@YingrongChen Yingrong Chen (YingrongChen) commented Aug 6, 2026

Copy link
Copy Markdown
Member

What this adds

Two LCU-oriented state preparations carved out of the SOSSA branch (#539), plus the Q# they rest on:

File Purpose
state_preparation/alias_sampling.py AliasSamplingStatePreparation — coherent alias-sampling PREPARE oracle (Babbush et al. 2018)
state_preparation/qrom_state_prep.py QROMStatePreparation — QROM lookup with phase-gradient rotations
qsharp/src/AliasSamplingStatePrep.qs Alias-sampling prepare, including the conditional/free-rider variants
qsharp/src/QROMStatePrep.qs QROM state prep and RyViaPhaseGradient
qsharp/src/PhaseGradient.qs Phase-gradient register preparation and addition
qsharp/src/SelectSwap.qs SELECT-SWAP (QROAM) lookup, 1D and 2D

plus registrations in algorithms/registry.py and exports in state_preparation/__init__.py, and tests for both preparations.

Ten files, pure addition, no existing file rewritten.

Relationship to the other branches

This is a parallel sibling off main, not a stack. It was originally carved alongside #617 and carried a copy of that PR's shared infrastructure so the two would merge cleanly. #617 has since merged, so all of that is now upstream and has been dropped from this branch: the Adaptive_RIF default in utils/qsharp/__init__.py, UnaryIteration.qs, the conftest.py fixtures, the Babbush2018 bibliography entry and the Base-profile test fixtures are all on main already. The branch has been rebased onto current main.

SelectSwap.qs imports QDKChemistry.Utils.UnaryIteration.UnaryIteration for Select2DLoad; that file now comes from main.

Note on register widths

Alias sampling is a PREPARE oracle: its Q# callable acts on the whole working register [index | uniform | flag | qromOutput], width 2n + 2mu + 1, not on the index qubits alone. The QPE, Hadamard-test and Euler builders all apply a state preparation to the system register by itself, so this operation is not a drop-in initial-state preparation for those paths — it is intended for LCU/qubitization, where the caller owns the full register. QROMStatePreparation allocates its phase-gradient ancillas internally and does act on the system register alone.

The tests here drive the alias-sampling circuit with its full width directly, so they exercise the intended contract.

Validation

_core.cp312-win_amd64.pyd on this machine predates main's CubeGenerator, and no wheel or CI artifact is available to replace it, so the suite could not be executed locally against the rebased base. CI is the oracle for test results here.

Verified locally: ruff check and format --check clean (the one remaining finding, RUF100 in remote/cache/__init__.py, is pre-existing on main and untouched here), and a syntax check over the changed Python files.

Copilot AI left a comment

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.

Pull request overview

This PR extracts and introduces an alias-sampling / QROM-based state-preparation stack into QDK Chemistry, adding the corresponding Q# implementations, Python algorithm wrappers, and functional simulator-backed tests, plus documentation updates and registry wiring.

Changes:

  • Added Q# implementations for SELECT-SWAP QROM loading, phase-gradient rotations, alias-sampling PREPARE, and QROM SBM state preparation.
  • Added Python algorithm wrappers (AliasSamplingStatePreparation, QROMStatePreparation) and functional tests that exercise the Q# operations via qdk.Context.
  • Updated algorithm exports/registry plus user documentation and examples to expose the new state-prep options.

Reviewed changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
python/tests/test_utils_select_swap.py Adds functional tests for SELECT-SWAP correctness and phase-oracle agreement.
python/tests/test_state_preparation_qrom.py Adds end-to-end tests for QROM SBM state prep and phase-gradient rotations.
python/tests/test_state_preparation_alias.py Adds end-to-end tests for alias sampling (1-norm PREPARE) and conditional signed variant.
python/tests/test_qsharp_context.py Updates Base-profile omission classification to include newly added Q# modules.
python/src/qdk_chemistry/utils/qsharp/src/SelectSwap.qs Introduces Q# SELECT-SWAP QROM data-loading network (1D/2D) and test wrappers.
python/src/qdk_chemistry/utils/qsharp/src/QROMStatePrep.qs Introduces Q# QROM SBM state preparation and sign correction.
python/src/qdk_chemistry/utils/qsharp/src/PhaseGradient.qs Introduces Q# phase-gradient helpers for Ry/Rz via addition and test wrappers.
python/src/qdk_chemistry/utils/qsharp/src/AliasSamplingStatePrep.qs Introduces Q# alias-sampling PREPARE plus conditional signed/free-rider variant and wrappers.
python/src/qdk_chemistry/algorithms/state_preparation/qrom_state_prep.py Adds Python QROMStatePreparation wrapper and settings plumbing.
python/src/qdk_chemistry/algorithms/state_preparation/alias_sampling.py Adds Python AliasSamplingStatePreparation wrapper and settings/input validation.
python/src/qdk_chemistry/algorithms/state_preparation/init.py Re-exports new state-prep algorithms from the package.
python/src/qdk_chemistry/algorithms/registry.py Registers the two new state-prep algorithms in the global algorithm registry.
docs/source/user/comprehensive/algorithms/state_preparation.rst Documents the new alias-sampling and QROM state-prep methods and settings.
docs/source/_static/examples/python/state_preparation.py Updates example output to show the new available state-prep implementations.
Suppressed comments (1)

python/src/qdk_chemistry/utils/qsharp/src/PhaseGradient.qs:23

  • Typo in doc comment: "Idealy" → "Ideally".
    /// Idealy this is prepared at the beginning of a circuit and reused throughout.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread python/src/qdk_chemistry/utils/qsharp/src/SelectSwap.qs
Comment thread python/src/qdk_chemistry/utils/qsharp/src/AliasSamplingStatePrep.qs
Comment thread python/src/qdk_chemistry/utils/qsharp/src/PhaseGradient.qs Outdated
Comment thread python/src/qdk_chemistry/algorithms/state_preparation/alias_sampling.py Outdated
Comment thread python/src/qdk_chemistry/algorithms/state_preparation/qrom_state_prep.py Outdated
Comment thread python/src/qdk_chemistry/utils/qsharp/src/QROMStatePrep.qs
Copilot AI review requested due to automatic review settings August 24, 2026 19:50

Copilot AI left a comment

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.

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated no new comments.

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

python/src/qdk_chemistry/algorithms/state_preparation/alias_sampling.py:150

  • The Python wrapper pads the coefficient vector up to the next power of two before passing it into the Q# alias-sampling implementation. That changes nCoeffs seen by AliasSamplingPrepare (it uses Length(params.coefficients) and PrepareUniformSuperposition(nCoeffs, ...)), which can change the prepared distribution / discretization behavior compared to the intended “L coefficients” semantics.

Since the Q# implementation already handles non-power-of-two lengths via nPadded = 1 <<< nIndexQubits and unreachable padded rows, the Python wrapper should pass the original coefficient list unchanged and let Q# handle the padding internally.

        num_index_qubits = math.ceil(math.log2(len(coefficients))) if len(coefficients) > 1 else 1
        padded_len = 1 << num_index_qubits
        if len(coefficients) < padded_len:
            coefficients = coefficients + [0.0] * (padded_len - len(coefficients))
        bits_precision = self.bits_precision

python/src/qdk_chemistry/utils/qsharp/src/SelectSwap.qs:225

  • Std.Arrays.Padded uses the sign of the first argument to choose head-vs-tail padding. Using Padded(-2^nRequired, ...) relies on precedence/reader interpretation of -2^nRequired; adding parentheses makes it unambiguous that the intent is tail-padding to length 2^nRequired.
    internal function CreatePaddedData(data : Bool[][], nRequired : Int, m : Int, k : Int) : Bool[][] {
        let dataPadded = Padded(-2^nRequired, [false, size = m], data);

Comment thread docs/source/user/comprehensive/algorithms/state_preparation.rst Outdated
Comment thread python/src/qdk_chemistry/algorithms/state_preparation/alias_sampling.py Outdated
Comment thread python/src/qdk_chemistry/utils/qsharp/src/QROMStatePrep.qs Outdated
Comment thread python/src/qdk_chemistry/algorithms/state_preparation/qrom_state_prep.py Outdated
Comment thread python/src/qdk_chemistry/utils/qsharp/src/PhaseGradient.qs Outdated
Comment thread python/src/qdk_chemistry/utils/qsharp/src/PhaseGradient.qs Outdated
Comment thread python/tests/test_state_preparation_alias.py Outdated
Comment thread python/tests/test_state_preparation_qrom.py
Comment thread python/src/qdk_chemistry/utils/qsharp/src/PhaseGradient.qs Outdated
Comment thread docs/source/_static/examples/python/state_preparation.py
Copilot AI review requested due to automatic review settings August 25, 2026 00:21

Copilot AI left a comment

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.

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated no new comments.

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

python/src/qdk_chemistry/utils/qsharp/src/AliasSamplingStatePrep.qs:89

  • DiscretizedProbabilityDistribution divides by total without guarding against the all-zero case (total == 0.0). If coefficients are all zero, AbsD(coefficients[i]) / total will divide by zero and crash instead of failing fast with a clear error.

Even though Python-side callers validate inputs, this is a public utility function inside the Q# project and can be reached from Q# callers/tests directly, so it should defensively reject an all-zero vector.

        mutable scaledTotal = 0;
        for i in 0..nCoeffs - 1 {
            let scaled = Round(AbsD(coefficients[i]) / total * IntAsDouble(targetTotal));
            set scaledProbs += [scaled];
            set scaledTotal += scaled;

python/src/qdk_chemistry/utils/qsharp/src/SelectSwap.qs:137

  • In WithSelectSwap, the numSwapBits == 0 branch calls Select(data, address, output) even though DimensionsForSelect computed nRequired and addressFitted is already available. If address is longer than required, passing the full register here can violate Select's expected address width. Using addressFitted keeps behavior consistent with SelectSwap's no-swap path.
        if numSwapBits == 0 {
            use output = Qubit[m];
            within {
                Select(data, address, output);
            } apply {

Copilot AI review requested due to automatic review settings August 25, 2026 00:31

Copilot AI left a comment

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.

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated no new comments.

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

python/src/qdk_chemistry/utils/qsharp/src/AliasSamplingStatePrep.qs:89

  • DiscretizedProbabilityDistribution divides by total without guarding against the all-zero input case. If coefficients are all 0.0, total becomes 0.0 and the scaling step hits a divide-by-zero, producing invalid keep/alias tables (or a runtime failure) rather than a clear error.

Even though the Python wrapper rejects all-zero vectors, this Q# helper is callable directly (and is reused by the conditional variant), so it should fail fast with a clear message when total == 0.0.

            let scaled = Round(AbsD(coefficients[i]) / total * IntAsDouble(targetTotal));

python/tests/test_state_preparation_qrom.py:51

  • This docstring claims the prepared basis is in “Q# little-endian order (qubit k = bit k)”, but the rest of this file treats dump_machine().as_dense_state() as big-endian (qubit 0 = MSB), and _build_expected_from_amplitudes also fills expected[j] without any bit-reversal.

To avoid confusion for future maintenance, the docstring should describe the ordering actually used for the comparison (i.e., the dump_machine/as_dense_state ordering).

    The QROM SBM decomposition prepares Σ_j (a_j/||a||) |j⟩ where j indexes
    the computational basis in Q# little-endian order (qubit k = bit k).
    """

Copilot AI review requested due to automatic review settings August 25, 2026 15:19

Copilot AI left a comment

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.

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated 4 comments.

Comment thread python/src/qdk_chemistry/utils/qsharp/src/AliasSamplingStatePrep.qs
Comment thread python/src/qdk_chemistry/utils/qsharp/src/QROMStatePrep.qs Outdated
Comment thread python/tests/test_state_preparation_alias.py Outdated
Comment thread python/tests/test_state_preparation_alias.py Outdated
Copilot AI review requested due to automatic review settings August 27, 2026 17:04

Copilot AI left a comment

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.

Pull request overview

Copilot reviewed 30 out of 30 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

docs/source/user/comprehensive/algorithms/hamiltonian_unitary_builder.rst:361

  • Both example snippets are under a .. tab:: Python API directive, which will render as two tabs with the same label (or otherwise be ambiguous) in the generated docs. Give the alias-sampling snippet a distinct tab label so readers can tell which is which.
.. tab:: Python API

Yingrong Chen (YingrongChen) added a commit that referenced this pull request Aug 27, 2026
Brings feature/cyr/sossa up to yingrongchen-alias-sampling-qrom-state-prep
at 3e6053e, so #539 can be retargeted onto #621 and reviewed as the
incremental SOSSA diff rather than as SOSSA plus all of alias sampling.

The previous merge (ca88541) captured 4ab6221; the alias branch has
moved 42 commits since, including two merges of main.

Two conflicts, both add/add registration lists, both resolved as unions:

- controlled_circuit_mapper/__init__.py: each side added a different mapper
  import and its __all__ entries. Kept both; __all__ is exactly the nine
  imported names.
- registry.py: each side registered a different mapper. Kept both, preserving
  each side's relative order, since registration order is load-bearing here
  (DensePureStatePreparation must precede SparseIsometryStatePreparation).

Two further breakages had NO conflict markers and would have shipped silently
had the merge been trusted at face value:

- PhaseGradient.qs: #621 deleted MakePhaseGradientAncillaPrep in ef8f189
  under a "Dead code" heading. It is dead on that branch but live on this one:
  sossa_mapper.get_ancilla_prep_op calls it, and SOSSAWalk.qs imports it. Git
  took the deletion cleanly because this side never touched that region.
  Restored verbatim, with a note recording why it must stay.
- PrepSelPrep.qs: #621 changed PrepSelPrep's 4th parameter from
  ancillaRegister to prepareRegister and added numSelectQubits, because an
  alias-sampling PREPARE register is wider than the select index. PSPWalk on
  this side still called it with four arguments. PSPWalk keeps its own arity,
  since MakeControlledPSPWalkOp and TestPSPWalkOnBasisState both depend on it;
  only its two internal calls pass the new argument, as
  Length(ancillaRegister), which is what merge-base computed as
  numAncillaQubits and so preserves the previous semantics exactly.

Verified: Q# project compiles; all 57 distinct QSHARP_UTILS.<ns>.<name>
references from Python resolve against the compiled context, with the checker
calibrated in both directions; ruff check and ruff format clean over 326 files.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Yingrong Chen (YingrongChen) added a commit that referenced this pull request Aug 27, 2026
A previous merge of the alias-sampling branch (ca88541) dropped

    from qdk_chemistry.data.registry import available_dataclasses, get_dataclass_type, register_dataclass

from data/__init__.py while leaving all three names in __all__. __all__ is a
list of strings, so the exports stayed *claimed* but unbound, and
`from qdk_chemistry.data import get_dataclass_type` raised ImportError.

This is why Build and Test was already red before this branch's merge, on
every platform: test_dataclass_registry.py and test_plugins.py failed at
import. The error named the test modules, not the merge that caused it.

Both main and #621 have the import; only the merge result lacked it, which
makes it a third instance of the pattern this branch has now hit twice in Q#
(PhaseGradient.qs, PrepSelPrep.qs): a change that is correct on each side
individually, silently broken by the merge, with no conflict marker anywhere.

Swept all 41 __init__.py files in the package for __all__ entries that the
module never binds; after this fix there are none. The checker was mutation-
tested against ca88541's actual file, where it reports exactly the three
missing names, so it is known to catch this defect rather than merely to pass.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot AI left a comment

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.

Pull request overview

Copilot reviewed 16 out of 16 changed files in this pull request and generated 4 comments.

Suppressed comments (6)

python/src/qdk_chemistry/utils/qsharp/src/PhaseGradient.qs:7

  • Doc comment has an extra trailing ///, which will render oddly in generated docs and makes the line inconsistent with the rest of the file.
/// Implements Ry and Rz rotations via phase gradient addition.///

python/src/qdk_chemistry/utils/qsharp/src/PhaseGradient.qs:23

  • Typo in doc comment: "Idealy" → "Ideally".
    /// Idealy this is prepared at the beginning of a circuit and reused throughout.

python/src/qdk_chemistry/utils/qsharp/src/AliasSamplingStatePrep.qs:55

  • DiscretizedProbabilityDistribution divides by total without guarding the all-zero case, which will cause a division-by-zero (and then invalid alias tables) if all coefficients are 0.
        mutable total = 0.0;
        for i in 0..nCoeffs - 1 {
            set total += AbsD(coefficients[i]);
        }

python/src/qdk_chemistry/utils/qsharp/src/SelectSwap.qs:80

  • Select2DLoad computes k = nRequired - numSwapBits without checking numSwapBits <= nRequired, which can produce a negative partition size and unclear runtime failures. Add the same guard used in SelectSwap.
                let (n, nRequired) = DimensionsForSelect(data[0], innerAddress);
                let innerAddressFitted = innerAddress[...nRequired - 1];

                let m = Length(data[0][0]);
                let l = numSwapBits;

python/src/qdk_chemistry/utils/qsharp/src/SelectSwap.qs:100

  • In the Select2DLoad adjoint, nRequired - numSwapBits is used without checking numSwapBits <= nRequired (and data[0] is used without checking data is non-empty). Add the same guards as the body so invalid inputs fail with a clear error.
            let (n, nRequired) = DimensionsForSelect(data[0], innerAddress);

            let mapOne : (Int -> Bool[][]) = (index) -> {
                CreatePaddedData(data[index], nRequired, Length(data[index][0]), nRequired - numSwapBits)
            };

python/src/qdk_chemistry/algorithms/state_preparation/alias_sampling.py:129

  • The bits_precision setter allows setting non-positive values after construction, which can later produce invalid Q# register slicing / shifts. It should enforce the same validation as __init__.
    @bits_precision.setter
    def bits_precision(self, value: int) -> None:
        self._bits_precision = value

Comment thread python/src/qdk_chemistry/utils/qsharp/src/PhaseGradient.qs
Comment thread python/src/qdk_chemistry/utils/qsharp/src/SelectSwap.qs Outdated
Comment thread python/src/qdk_chemistry/algorithms/state_preparation/qrom_state_prep.py Outdated
Comment thread python/src/qdk_chemistry/algorithms/state_preparation/alias_sampling.py Outdated

Copilot AI left a comment

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.

Pull request overview

Copilot reviewed 30 out of 30 changed files in this pull request and generated 7 comments.

Suppressed comments (4)

python/src/qdk_chemistry/utils/qsharp/src/PhaseGradient.qs:23

  • Typo in doc comment: "Idealy" → "Ideally".
    /// Ideally this is prepared at the beginning of a circuit and reused throughout.

python/tests/test_state_preparation_alias.py:177

  • With bits_precision = 6 and free-rider bits, the conditional-alias test allocates a ~23-qubit register and dumps a 2^23 statevector, which is likely too expensive for unit-test CI. Reducing bits_precision (and/or free-rider width) keeps the test meaningful while making it tractable.
        expected_probs = abs_coeffs / np.sum(abs_coeffs)

python/tests/test_state_preparation_alias.py:149

  • _compute_conditional_marginal_probs formats each basis index as a binary string inside a loop over the full statevector. For the current parameters this can be millions of iterations and is very slow. This can be vectorized using shift/mask extraction of the BE fields plus a small bit-reversal lookup table, and np.add.at for accumulation.
            marginal_probs,
            [0.5, 0.3, 0.2, 0.0],
            atol=_alias_atol(len(coefficients), bits_precision),
        )

python/src/qdk_chemistry/utils/qsharp/src/SelectSwap.qs:99

  • The numSwapBits <= nRequired guard should also be applied in the adjoint implementation. Without it, calling Adjoint Select2DLoad with an out-of-range numSwapBits will still hit nRequired - numSwapBits and fail with a less clear runtime error.

        let addressBits = Ceiling(Lg(IntAsDouble(numInnerData)));
        for lambda in 0..addressBits - 1 {
            let cost = SelectSwapCost2D(lambda, numOuterData, numInnerData, numBits);

Comment thread python/src/qdk_chemistry/utils/qsharp/src/PhaseGradient.qs Outdated
Comment thread python/src/qdk_chemistry/utils/qsharp/src/AliasSamplingStatePrep.qs
Comment thread python/src/qdk_chemistry/utils/qsharp/src/SelectSwap.qs Outdated
Comment thread python/src/qdk_chemistry/utils/qsharp/src/SelectSwap.qs Outdated
Comment thread python/tests/test_state_preparation_alias.py
Comment thread python/tests/test_state_preparation_alias.py

Copilot AI left a comment

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.

Pull request overview

Copilot reviewed 30 out of 30 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

python/src/qdk_chemistry/utils/qsharp/src/CircuitComposition.qs:76

  • MakeSharedAncillaOp slices qs[Length(qs) - numShared...] without validating numShared. If numShared is negative or larger than the provided register, this will fail at runtime with an out-of-range slice, making errors harder to diagnose for callers composing circuits.
        (qs) => {
            within {
                if numShared > 0 {
                    prepareShared(qs[Length(qs) - numShared...]);
                }

Copilot AI left a comment

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.

Pull request overview

Copilot reviewed 33 out of 33 changed files in this pull request and generated 1 comment.

Suppressed comments (2)

Previously missed (1) — in code that hasn't changed since the last review.

python/src/qdk_chemistry/utils/qsharp/src/PhaseGradient.qs:134

  • MakeTestRyRoundtripOp currently places RyViaPhaseGradient inside the within block, so it gets auto-uncomputed immediately (because the apply block is empty). That makes the test trivially pass even if the adjoint implementation is wrong; it never exercises an explicit Ry followed by Adjoint Ry roundtrip.
            within {
                PreparePhaseGradientState(pg);
                RyViaPhaseGradient(qs[0], angle, pg);
            } apply {}

python/src/qdk_chemistry/utils/qsharp/src/QROMStatePrep.qs:18

  • ComputeSBMAngles uses Repeated(...), but this file doesn't import Std.Arrays.Repeated (only Any and Reversed). This can fail to compile depending on which namespaces are opened by default; please explicitly import Repeated (or qualify it) to make the dependency unambiguous.
    import Std.Arrays.Any;
    import Std.Arrays.Reversed;

Comment thread python/src/qdk_chemistry/utils/qsharp/src/SelectSwap.qs Outdated

Copilot AI left a comment

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.

Pull request overview

Copilot reviewed 33 out of 33 changed files in this pull request and generated 3 comments.

Suppressed comments (1)

python/src/qdk_chemistry/utils/qsharp/src/QROMStatePrep.qs:20

  • QROMStatePrep.qs uses Repeated(...) in ComputeSBMAngles, but the file does not import it. This will fail to compile unless Repeated is brought into scope explicitly.
    import Std.Arrays.Any;
    import Std.Arrays.Reversed;
    import Std.Canon.ApplyPauliFromBitString;
    import Std.Convert.IntAsBoolArray;

Comment thread python/src/qdk_chemistry/utils/qsharp/src/SelectSwap.qs Outdated
Comment thread python/src/qdk_chemistry/algorithms/circuit_mapper/psp_mapper.py

Copilot AI left a comment

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.

Pull request overview

Copilot reviewed 33 out of 33 changed files in this pull request and generated no new comments.

Suppressed comments (4)

Previously missed (1) — in code that hasn't changed since the last review.

python/src/qdk_chemistry/utils/qsharp/src/QROMStatePrep.qs:66

  • QROMStatePrepare can be called with numStateQubits == 0 or rotationBitPrecision == 0, which will cause out-of-range access (angleTree[1]) or invalid phase-gradient preparation. Add explicit guards so misuse fails with a clear message.
        let n = params.numStateQubits;
        let bRot = params.rotationBitPrecision;
        let angleTree = ComputeSBMAngles(params.amplitudes, n, bRot);

python/src/qdk_chemistry/utils/qsharp/src/PrepSelPrep.qs:116

  • PrepSelPrep slices prepareRegister[0..numSelectQubits - 1], which becomes 0..-1 when numSelectQubits == 0 (e.g., single-term LCU where num_prepare_ancillas = ceil(log2(1)) = 0). That range is invalid and will fail at runtime/compile-time depending on Q# semantics. Pass an empty control register when numSelectQubits is 0.
                    selectOp(prepareRegister[0..numSelectQubits - 1], targetRegister);

python/src/qdk_chemistry/utils/qsharp/src/QROMStatePrep.qs:19

  • ComputeSBMAngles uses Repeated(...) but this file does not import it, which will fail Q# compilation unless Repeated is in scope. Add import Std.Arrays.Repeated; alongside the other Std.Arrays imports.
    import Std.Arrays.Any;
    import Std.Arrays.Reversed;
    import Std.Canon.ApplyPauliFromBitString;

python/src/qdk_chemistry/utils/qsharp/src/PrepSelPrep.qs:128

  • Same numSelectQubits == 0 issue exists in the controlled specialization: slicing prepareRegister[0..numSelectQubits - 1] becomes 0..-1 and breaks single-term LCUs (and any PREPARE that provides ancilla but no index controls).
                Controlled selectOp(ctls, (prepareRegister[0..numSelectQubits - 1], targetRegister));

Copilot AI left a comment

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.

Pull request overview

Copilot reviewed 33 out of 33 changed files in this pull request and generated no new comments.

Suppressed comments (5)

Previously missed (1) — in code that hasn't changed since the last review.

python/src/qdk_chemistry/utils/qsharp/src/CircuitComposition.qs:80

  • MakeSharedAncillaOp slices qs[Length(qs) - numShared...] when numShared > 0 but does not validate numShared against the actual register length. If a caller passes numShared larger than Length(qs) (or negative), this will throw a slice error. Add an explicit bounds check and fail with a clear message.
    function MakeSharedAncillaOp(
        op : Qubit[] => Unit is Adj + Ctl,
        prepareShared : Qubit[] => Unit is Adj + Ctl,
        numShared : Int
    ) : Qubit[] => Unit is Adj + Ctl {
        (qs) => {
            within {
                if numShared > 0 {
                    prepareShared(qs[Length(qs) - numShared...]);
                }

python/src/qdk_chemistry/utils/qsharp/src/PrepSelPrep.qs:117

  • PrepSelPrep slices prepareRegister[0..numSelectQubits - 1] without validating numSelectQubits. If numSelectQubits is negative or larger than the register, this will fail at runtime with an unhelpful slice error. Add an explicit range check and fail early with a clear message (only in the non-empty PREPARE case).
                within {
                    prepareOp(prepareRegister);
                } apply {
                    selectOp(prepareRegister[0..numSelectQubits - 1], targetRegister);
                }

python/src/qdk_chemistry/algorithms/state_preparation/state_preparation.py:116

  • _dense_state_vector assumes wavefunction.get_active_determinants() is non-empty and immediately indexes determinants[0]. A Wavefunction can be empty (see existing utilities that construct Wavefunction([])), and inconsistent inputs could also produce coefficients with no determinants, leading to an IndexError instead of a clear ValueError. Add an explicit empty-determinant check.
        determinants = wavefunction.get_active_determinants()
        num_bits = wavefunction.get_configuration_set().num_modes() * determinants[0].bits_per_mode()
        num_qubits = max(num_bits, 1)

docs/source/user/comprehensive/algorithms/hamiltonian_unitary_builder.rst:352

  • The example uses two standalone .. tab:: Python API directives without a surrounding .. tabs::/tab-set container (and with duplicate tab titles). This is likely invalid reStructuredText for the tabs extension and can break the docs build. Consider using plain .. literalinclude:: blocks here instead (or wrap both tabs in the correct container with distinct titles).
.. tab:: Python API

   .. literalinclude:: ../../../_static/examples/python/hamiltonian_unitary_builder.py

python/src/qdk_chemistry/utils/qsharp/src/PrepSelPrep.qs:129

  • The controlled specialization also slices prepareRegister[0..numSelectQubits - 1] without validating numSelectQubits. Even if callers are well-behaved today, this is a public utility and should guard against invalid inputs to avoid runtime slice failures.
            if (Length(prepareRegister) == 0) {
                Controlled selectOp(ctls, ([], targetRegister));
            } else {
                prepareOp(prepareRegister);
                Controlled selectOp(ctls, (prepareRegister[0..numSelectQubits - 1], targetRegister));
                Adjoint prepareOp(prepareRegister);

Copilot AI left a comment

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.

Pull request overview

Copilot reviewed 33 out of 33 changed files in this pull request and generated no new comments.

Suppressed comments (3)

python/src/qdk_chemistry/algorithms/state_preparation/state_preparation.py:116

  • _dense_state_vector forces num_qubits to be at least 1 (max(num_bits, 1)), which breaks valid 0-qubit wavefunctions (e.g., the single-term LCU PREPARE case builds a 0-mode wavefunction in LCUBuilder._build_prepare). This will incorrectly inflate the PREPARE register width and can make block-encoding/QPE wiring inconsistent.
        determinants = wavefunction.get_active_determinants()
        num_bits = wavefunction.get_configuration_set().num_modes() * determinants[0].bits_per_mode()
        num_qubits = max(num_bits, 1)

python/src/qdk_chemistry/utils/qsharp/src/QROMStatePrep.qs:22

  • QROMStatePrep.qs uses Repeated(...) (e.g., lines 168, 175, 186) but does not import Std.Arrays.Repeated or qualify it. This will fail to compile the Q# source.
    import Std.Arrays.Any;
    import Std.Arrays.Reversed;
    import Std.Canon.ApplyPauliFromBitString;
    import Std.Convert.IntAsBoolArray;
    import Std.Convert.IntAsDouble;
    import Std.Math.ArcCos;

docs/source/user/comprehensive/algorithms/hamiltonian_unitary_builder.rst:363

  • The .. tab:: directives added in the Example section are not nested under a .. tabs:: container (sphinx-tabs) and are outdented at top level, which will break Sphinx parsing. If tabs are intended, wrap both examples in .. tabs:: and use distinct tab labels; otherwise remove the .. tab:: lines and keep the .. literalinclude:: blocks directly.
.. rubric:: Example

.. tab:: Python API

   .. literalinclude:: ../../../_static/examples/python/hamiltonian_unitary_builder.py
      :language: python
      :start-after: # start-cell-run-lcu
      :end-before: # end-cell-run-lcu

To use alias sampling for the PREPARE oracle, pass an
:class:`~qdk_chemistry.data.AlgorithmRef` to the
:class:`~qdk_chemistry.algorithms.circuit_mapper.psp_mapper.PSPMapper`:

.. tab:: Python API

   .. literalinclude:: ../../../_static/examples/python/hamiltonian_unitary_builder.py

Copilot AI left a comment

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.

Pull request overview

Copilot reviewed 33 out of 33 changed files in this pull request and generated no new comments.

Suppressed comments (2)

python/src/qdk_chemistry/utils/qsharp/src/PrepSelPrep.qs:116

  • PrepSelPrep slices prepareRegister[0..numSelectQubits - 1] without handling numSelectQubits == 0. For single-term LCUs (num_prepare_ancillas == 0), numSelectQubits can be 0 while prepareRegister may be non-empty, which makes this range invalid and will fail at runtime. Pass an empty control register when numSelectQubits is 0.
                    selectOp(prepareRegister[0..numSelectQubits - 1], targetRegister);

python/src/qdk_chemistry/utils/qsharp/src/PrepSelPrep.qs:128

  • Same issue in the controlled specialization: prepareRegister[0..numSelectQubits - 1] is invalid when numSelectQubits == 0. Use an empty control register for SELECT in that case so single-term LCUs (and any other 0-control SELECT) don't crash.
                Controlled selectOp(ctls, (prepareRegister[0..numSelectQubits - 1], targetRegister));

Copilot AI left a comment

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.

Pull request overview

Copilot reviewed 33 out of 33 changed files in this pull request and generated 1 comment.

Suppressed comments (2)

python/src/qdk_chemistry/algorithms/circuit_mapper/psp_mapper.py:164

  • build_prep_ops() always tries to materialize a PREPARE circuit when lcu.prepare is present, but LCUBuilder sets num_prepare_ancillas = ceil(log2(num_terms)), so a single-term Hamiltonian has num_prepare_ancillas == 0. In that case SELECT has no control qubits and the block encoding should use an empty PREPARE register; the current logic will instead embed a non-empty PREPARE circuit and pass numSelectQubits=0, which can lead to an invalid SELECT-register slice in Q# and/or incorrect register sizing.
        lcu, _ = self.resolve_lcu(container)
        if lcu.prepare is None:
            return QSHARP_UTILS.PrepSelPrep.NoOpPrepare, 0, 0

        prepare_algorithm = self._create_nested("prepare")

python/src/qdk_chemistry/algorithms/state_preparation/state_preparation.py:117

  • _dense_state_vector() forces num_qubits >= 1 via max(num_bits, 1). This changes the meaning of a valid 0-mode/0-qubit wavefunction (e.g., the single-term LCU PREPARE state) by densifying it into a 1-qubit (length-2) statevector, which can cascade into incorrect register widths and downstream slicing issues.
        determinants = wavefunction.get_active_determinants()
        num_bits = wavefunction.get_configuration_set().num_modes() * determinants[0].bits_per_mode()
        num_qubits = max(num_bits, 1)
        if num_qubits > 32:

Comment thread python/src/qdk_chemistry/utils/qsharp/src/PhaseGradient.qs

Copilot AI left a comment

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.

Pull request overview

Copilot reviewed 33 out of 33 changed files in this pull request and generated 2 comments.

Comment on lines +79 to +83
// Distribute leftover units to the largest remainders first, ties by ascending index.
// Flooring keeps the residual non-negative, so bar heights only increase.
mutable residual = targetTotal - scaledTotal;
for _ in 1..residual {
mutable bestIdx = -1;
Comment on lines +154 to +161
from qdk_chemistry.data import AlgorithmRef

# Load the PREPARE amplitudes with alias sampling instead of dense state preparation
psp_mapper = create(
"circuit_mapper",
"prepare_select_prepare",
prepare=AlgorithmRef("state_prep", "alias_sampling", bits_precision=6),
)
Adds two PREPARE-style state preparations alongside the existing dense and
sparse ones. Alias sampling loads a discretized distribution with a keep/alt
table and a coherent comparison; QROM state preparation loads Ry rotation
angles from a QROM and applies them through a shared phase gradient register.
Both index their amplitudes in the Jordan-Wigner basis.

Supporting Q# utilities: select-swap lookup, phase gradient preparation, and
the plumbing that lets phase estimation share the gradient register with the
state preparation.

This collapses the branch's development history, which carried five merges
from main that a linear rebase cannot replay. The tree is unchanged.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Route alias sampling, QROM and dense_pure_state through a single
dense_coefficients helper so each coefficient lands at its
determinant-derived index instead of its position in the list.

Give the unary QPE builder a shared-ancilla segment so a state prep or
block encoding that needs a caller-owned register (QROM's phase
gradient) gets it prepared once, and drop the shared-ancilla plumbing
from the prepare/select/prepare mappers, which supply none.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Replace Circuit's shared-ancilla count/callable pair with a single
serializable `num_phase_gradient_ancillas: int`. Callers that previously
received a prep operation now invoke the Q# op themselves, so the whole
declaration participates in serialization and content hashing. Bumps the
circuit serialization version 0.1.1 -> 0.1.2 (patch, so existing artifacts
still load).

Hoist the duplicated dense-coefficient scatter into
`StatePreparation.dense_state_vector`, shared by the dense, alias and QROM
preparations.

Guard `MakeSharedAncillaOp` against `numShared == 0`:
`PreparePhaseGradientState` applies `X(qs[n - 1])`, so an empty slice failed
with "index out of range: -1".

Fix the alias sampling LCU energy test, which could not pass on any platform.
With coefficients [0.25, 0.625, 0.125] the eigenphase landed near a phase-bin
boundary and the nearest bin center was 0.0400 off, twice the 0.02 tolerance.
Retuned to [0.1875, 0.25, 0.5625], which stays exact in the alias table
(multiples of 1/16) and sits on a bin center, leaving 0.00550 error.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
`MakeUnaryQPECircuit` invoked `prepareSharedOp` unconditionally, so a caller
passing a real preparation with `numSharedAncillas = 0` hit
`X(phaseGradient[-1])`. Only prepare when the shared register is non-empty.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Move `num_phase_gradient_ancillas` off `Circuit`'s constructor and into a frozen
`CircuitMetadata` dataclass, so later per-subroutine facts can be added without
growing the signature each time. Follows `QsharpFactoryData`: a plain
declaration, with serialization handled explicitly by `Circuit` alongside its
other fields.

The metadata is written only when it departs from the default, so circuits that
declare none serialize and hash exactly as before. Serialization stays at 0.1.2.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Make `dense_state_vector` private, since only state prep subclasses use it.
`AliasSamplingStatePreparation._sampling_weights` becomes a classmethod so the
call stays inside the hierarchy.

Name the PREPARE oracle in the phase gradient rejection and say what to do about
it, and cover every guard in `build_prep_ops` — none of them had a test. The
phase gradient case runs the real path through `prepare="qrom"`; the rest use a
stub PREPARE, so they cost nothing to run.

The free-rider test asserted the same marginals as the conditional sweep and
never looked at the free-rider bits. Since `ConditionalAliasSamplingPrepare`
just delegates to the free-rider operation with empty data, the sweep already
covers that sampling, so assert the new thing instead: the free rider depends
only on the condition, so its register comes out definite rather than sampled.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Both scatter coefficients onto determinant-derived indices via
_dense_state_vector, so their index registers use the same
occupation-number convention as dense_pure_state, which already tags the
encoding. Leaving it unset let euler_builder's mismatch guard pass
silently, since it only compares when both sides are non-None.

Also merge _build_pauli_select_op into build_select_ops, its only caller,
and describe the encoding convention in the StatePreparation note.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The occupation-number scatter in _dense_state_vector is what fixes the
basis, so say so there rather than in the class note.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Phase estimation handed the state preparation a register sized from the
Hamiltonian alone, so a PREPARE oracle that owns extra ancilla -- QROM's
phase gradient, alias sampling's garbage -- indexed past the end and only
failed deep inside Q#. Check the declared width up front in all three
builders; the unary builder counts the shared gradient it supplies.

Also guard the two empty-input cases Copilot flagged: a zero-width phase
gradient register indexed phaseGradient[-1], and Select2DLoad read data[0]
before anything checked data was non-empty. Rename the shadowing
numSwapBits binding in SelectSwap while there.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The numSwapBits == 0 branch passed the raw address register while the
sibling branch and SelectSwap itself pass the fitted slice. The branch is
currently unreachable, so this is a consistency fix rather than a bug fix.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
num_qubits is the width of the register the op is applied to; scratch qubits a
circuit allocates internally are not counted. A resource estimate does count
them, so estimate > num_qubits is normal rather than a disagreement.

Overwriting num_qubits with the estimate stored a value the field is defined not
to hold, and made a read-only query mutate the circuit: a binary-encoding state
prep declaring 8 qubits became 11 once estimate() ran, so phase estimation then
rejected it for exceeding the 8-qubit system register it fits exactly.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Keep telling the caller when a resource estimate counts more qubits than the
circuit declares, but only as information: the difference is expected whenever a
circuit allocates and deallocates ancillas internally, since num_qubits is the
width of the register the op is applied to.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot AI left a comment

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.

Pull request overview

Copilot reviewed 33 out of 33 changed files in this pull request and generated no new comments.

Suppressed comments (3)

python/src/qdk_chemistry/utils/qsharp/src/QROMStatePrep.qs:18

  • ComputeSBMAngles uses Repeated(...) but this module doesn't import or qualify Repeated, so the Q# project will fail to compile. Add the missing Std.Arrays.Repeated import (or qualify the calls) near the top of the file.
    import Std.Arrays.Any;
    import Std.Arrays.Reversed;

python/src/qdk_chemistry/utils/qsharp/src/PrepSelPrep.qs:117

  • PrepSelPrep slices prepareRegister[0..numSelectQubits - 1] but never validates numSelectQubits against Length(prepareRegister). If the caller passes an out-of-range value, this will fail at runtime (or produce unintended controls). Add an explicit guard so failures are immediate and clear.
        body ... {
            if (Length(prepareRegister) == 0) {
                selectOp([], targetRegister);
            } else {
                within {
                    prepareOp(prepareRegister);
                } apply {
                    selectOp(prepareRegister[0..numSelectQubits - 1], targetRegister);
                }

python/src/qdk_chemistry/utils/qsharp/src/CircuitComposition.qs:76

  • MakeSharedAncillaOp does not validate numShared. If numShared is negative or larger than Length(qs), the slice qs[Length(qs) - numShared...] will fail with a low-level range error. Add explicit bounds checks so callers get a clear message.
    function MakeSharedAncillaOp(
        op : Qubit[] => Unit is Adj + Ctl,
        prepareShared : Qubit[] => Unit is Adj + Ctl,
        numShared : Int
    ) : Qubit[] => Unit is Adj + Ctl {
        (qs) => {
            within {
                if numShared > 0 {
                    prepareShared(qs[Length(qs) - numShared...]);
                }

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.

4 participants