SOSSA hamiltonian builder and circuit mapper - #539
SOSSA hamiltonian builder and circuit mapper#539Yingrong Chen (YingrongChen) wants to merge 456 commits into
Conversation
…RIF) Replace the single global Base Q# interpreter used for the vendored chemistry utilities with isolated qdk.Context environments, one per target profile. QSHARP_UTILS resolves against an active profile that defaults to Adaptive_RIF, so all builders now produce Adaptive_RIF circuits. - utils/qsharp/__init__.py: BASE/ADAPTIVE contexts + utils, get_qsharp_context, get_qsharp_utils, _ActiveQSharpUtils proxy (default Adaptive_RIF), and a use_qsharp_profile() context manager for downstream code that needs to switch profiles (e.g. Base for qir_to_qiskit). - Relocate the 10 utility .qs files under src/ and add qsharp.json so the Adaptive context loads them as a Q# project; the Base context concatenates them directly. - data/circuit.py, circuit_executor/qdk.py, circuit_mapper/pauli_sequence_mapper.py: route compile/circuit/run/estimate through each program's baked _qdk_context. - Tests: use isolated get_qsharp_context() instead of the global interpreter; assert on BASE_QSHARP_UTILS. This is the shared change extracted from #539 and #540; both stack on top of it. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 79394164-7583-4dfa-9e24-b3c3d5cbac4c
Replace the single global Base Q# interpreter used for the vendored chemistry utilities with a dedicated qdk.Context built with the Adaptive_RIF target profile. QSHARP_UTILS now resolves against that context, so all builders produce Adaptive_RIF circuits. - utils/qsharp/__init__.py: minimal get_qsharp_context() (default Adaptive_RIF) and QSHARP_UTILS. - Relocate the 10 utility .qs files under src/ and add qsharp.json so the context loads them as a Q# project. - data/circuit.py, circuit_executor/qdk.py, circuit_mapper/pauli_sequence_mapper.py: route compile/circuit/run/estimate through each program's baked _qdk_context. - Tests: use isolated get_qsharp_context() instead of the global interpreter. This is the shared change extracted from #539 and #540; both stack on top of it. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 79394164-7583-4dfa-9e24-b3c3d5cbac4c
Carves the alias-sampling / QROM state-preparation stack out of the SOSSA implementation (#539) into a standalone change that targets main directly. Adds: * `AliasSamplingStatePreparation` and `QROMStatePreparation` Python algorithms, their registry entries and package exports. * The Q# sources they compile against: `AliasSamplingStatePrep.qs`, `QROMStatePrep.qs`, `PhaseGradient.qs` and `SelectSwap.qs`. * `UnaryIteration.qs`, a hard compile-time dependency of `SelectSwap.Select2DLoad`, byte-identical to the copy in #617 so the two merges stay clean. * Unit tests for both state preparations, which execute on the sparse simulator. `AliasSamplingStatePrep.qs` branches on measurement results, which `TargetProfile.Base` rejects at load time. Since `create_qsharp_context()` previously defaulted to Base and loaded the whole vendored Q# project in one go, adding these sources would break every Q# test in the repository. The default profile therefore moves to `Adaptive_RIF`, and callers that explicitly ask for Base get a staged sub-project containing only the Base-legal sources. Test modules that convert QIR to Qiskit need measurement-free circuits, so they opt back in to Base through the new `use_base_qdk_ctx` fixture. `test_hadamard_test.py` pins exact shot counts, which adaptive lowering shifts, so it compiles under Base too -- via a module-scoped fixture, because a `Circuit` pins the context its Q# callable was built in and its benchmark circuits are built by a module-scoped fixture. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Carves the alias-sampling / QROM state-preparation stack out of the SOSSA implementation (#539) into a standalone change that targets main directly. Adds: * `AliasSamplingStatePreparation` and `QROMStatePreparation` Python algorithms, their registry entries and package exports. * The Q# sources they compile against: `AliasSamplingStatePrep.qs`, `QROMStatePrep.qs`, `PhaseGradient.qs` and `SelectSwap.qs`. * `UnaryIteration.qs`, a hard compile-time dependency of `SelectSwap.Select2DLoad`, byte-identical to the copy in #617 so the two merges stay clean. * Unit tests for both state preparations, which execute on the sparse simulator. `AliasSamplingStatePrep.qs` branches on measurement results, which `TargetProfile.Base` rejects at load time. Since `create_qsharp_context()` previously defaulted to Base and loaded the whole vendored Q# project in one go, adding these sources would break every Q# test in the repository. The default profile therefore moves to `Adaptive_RIF`, and callers that explicitly ask for Base get a staged sub-project containing only the Base-legal sources. Test modules that convert QIR to Qiskit need measurement-free circuits, and `test_hadamard_test.py` pins exact shot counts that adaptive lowering shifts, so both opt back in to Base through the new fixtures in `conftest.py`. Every file shared with #617 is byte-identical to its copy there, so whichever of the two merges second is a no-op for those files. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Carves the alias-sampling / QROM state-preparation stack out of the SOSSA implementation (#539) into a standalone change that targets main directly. Adds: * `AliasSamplingStatePreparation` and `QROMStatePreparation` Python algorithms, their registry entries and package exports. * The Q# sources they compile against: `AliasSamplingStatePrep.qs`, `QROMStatePrep.qs`, `PhaseGradient.qs` and `SelectSwap.qs`. * `UnaryIteration.qs`, a hard compile-time dependency of `SelectSwap.Select2DLoad`, byte-identical to the copy in #617 so the two merges stay clean. * Unit tests for both state preparations, which execute on the sparse simulator. `AliasSamplingStatePrep.qs` branches on measurement results, which `TargetProfile.Base` rejects at load time. Since `create_qsharp_context()` previously defaulted to Base and loaded the whole vendored Q# project in one go, adding these sources would break every Q# test in the repository. The default profile therefore moves to `Adaptive_RIF`, and callers that explicitly ask for Base get a staged sub-project containing only the Base-legal sources. Test modules that convert QIR to Qiskit need measurement-free circuits, and `test_hadamard_test.py` pins exact shot counts that adaptive lowering shifts, so both opt back in to Base through the new fixtures in `conftest.py`. Every file shared with #617 is byte-identical to its copy there, so whichever of the two merges second is a no-op for those files. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Validate numSwapBits in Select2DLoad, matching SelectSwap, so an out-of-range value reports the cause instead of a negative partition size. Correct the alias sampling docs to the 2-norm semantics the Python layer actually implements, drop the settings text the cleanup removed from the Python string, and retire the claim that prepare_select_prepare cannot supply the extra ancillas. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
MakeQROMStatePrepOp sliced a phase gradient register out of the caller's qubits without preparing it. Every caller reached through StatePreparation._qsharp_op (e.g. the unary phase estimation builder) hands over a freshly allocated register, so the multiplexed rotations read |0...0> instead of a gradient and the op silently returned |0...0> rather than the requested state. Measured fidelity 0.545 for [0.5, 0.3, 0.7, 0.1] at bRot=4; now 0.985 at bRot=4, 0.99999 at bRot=8 and 0.999997 at bRot=10, i.e. limited only by rotation precision. MakeQROMStatePrepCircuit and RunQROMStatePrep already wrapped the call this way; the op is now consistent with them. Also renames a test wrapper call to the existing Q# operation name, which was breaking collection of the 2D swap-path test. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The empty __all__ is a repo-wide convention (12+ modules on main use it) that stops sphinx autosummary from re-documenting names a module merely imports. Dropping it made autosummary document Circuit, Settings and Wavefunction inside qdk_chemistry.algorithms.state_preparation, producing 7 warnings that fail the docs build: duplicate object description of qdk_chemistry.data.circuit.Circuit duplicate object description of qdk_chemistry.data.Settings more than one target found for cross-reference 'Wavefunction' Every sibling module in the package already declares __all__; this was the only one missing it. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
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>
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>
The class docstring ended by saying the mapper "can also drive it via :meth:`build_walk_op`", but that method does not exist on the class. It was retired when #617 re-architected the mapper contract; the reference was introduced by this branch and survived because nothing executes a docstring and a bare role only breaks at documentation build time. Replaced with what the code actually does: the walk is composed inline during circuit construction, gated on the container resolving to a walk, and there is no separate walk-construction entry point to point at. Swept all 50 Python files this branch changes for bare :meth:/:attr:/:func: roles naming something absent from their own file; this was the only one. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🔵 Needs a closer look
The new examples/sossa_qre.ipynb contains runtime-breaking algorithm-name and attribute-access issues that would prevent the notebook (and its new test) from executing successfully.
Review details
Suppressed comments (3)
examples/sossa_qre.ipynb:189
- SOSSAWalkContainer exposes the block-encoding normalization as
container.normalization, not undercontainer.metadata(which is FactorizedHamiltonianMetadata). Using.metadata.normalizationwill raise AttributeError at runtime.
" hf_config = Configuration.canonical_hf_configuration(n_alpha, n_beta, num_orbitals)\n",
examples/sossa_qre.ipynb:426
metadatadoes not containnormalization; normalization is a property on the SOSSA walk container itself. Also, the qubit mapper variant name should be "sos" (not "sossa"). As written, this cell will fail at runtime.
"print(f\"System qubits = {n2_operator.get_container().num_qubits}\")\n",
examples/sossa_qre.ipynb:185
- The SOS mapper is registered as "sos" (SOSQubitMapper). Using "sossa" here will fail algorithm lookup and prevent the notebook from executing.
"\n",
" if num_queries is None:\n",
" sossa_walk = create(\"hamiltonian_unitary_builder\", \"sossa\").run(operator)\n",
" lambda_sos = sossa_walk.get_container().metadata.normalization\n",
- Files reviewed: 32/32 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
The new examples/sossa_qre.ipynb references a non-existent qubit-mapper variant ("sossa") and reads normalization from the wrong object (metadata), which will break notebook execution and the added notebook test.
Review details
Suppressed comments (4)
examples/sossa_qre.ipynb:179
- The notebook uses
create("qubit_mapper", "sossa"), but the SOS factorized-Hamiltonian qubit mapper is registered under the variant name"sos"(seeSOSQubitMapper.name()), so this cell will raise at runtime and the notebook test will fail.
" operator = create(\"qubit_mapper\", \"sossa\").run(\n",
examples/sossa_qre.ipynb:185
SOSSAWalkContainerstores the block-encoding normalization oncontainer.normalization, not oncontainer.metadata. Usingmetadata.normalizationwill raiseAttributeErrorwhen executing the notebook.
" lambda_sos = sossa_walk.get_container().metadata.normalization\n",
examples/sossa_qre.ipynb:419
- This cell also calls
create("qubit_mapper", "sossa"), but the qubit-mapper variant name is"sos"; leaving this as-is will break notebook execution.
"n2_operator = create(\"qubit_mapper\", \"sossa\").run(\n",
examples/sossa_qre.ipynb:426
create("hamiltonian_unitary_builder", "sossa").run(...).get_container().metadatareturnsFactorizedHamiltonianMetadata, which does not include the block-encoding normalization. The notebook then readsn2_metadata.normalizationand will fail; keep the walk container around and readnormalizationfrom it.
"n2_metadata = create(\"hamiltonian_unitary_builder\", \"sossa\").run(n2_operator).get_container().metadata\n",
"\n",
"print(f\"Block-encoding normalization Lambda = {n2_metadata.normalization:.6f} Hartree\")\n",
"print(f\"Sum-of-squares shift E_SOS = {n2_metadata.energy_shift:.6f} Hartree\")\n",
"print(f\"System qubits = {n2_operator.get_container().num_qubits}\")\n",
- Files reviewed: 32/32 changed files
- Comments generated: 0 new
- Review effort level: Lite
…chemistry into feature/cyr/sossa
There was a problem hiding this comment.
🔵 Needs a closer look
The SOSSA QRE notebook currently contains malformed indentation that will cause SyntaxError during execution, and there are phase-gradient precision consistency issues in the SOSSA circuit mapper that can break valid configurations.
Review details
Suppressed comments (6)
Previously missed (4) — in code that hasn't changed since the last review.
python/src/qdk_chemistry/algorithms/circuit_mapper/sossa_mapper.py:105
- When
outer_prepareisqrom, the outer PREPARE must share the same phase-gradient width as SELECT. Currently onlyallocate_phase_gradientis forced off;rotation_bit_precisionis left at the QROM state's default (often 10), which can disagree withSOSSAMapper.rotation_bit_precisionand trigger a layout/metadata mismatch.
python/src/qdk_chemistry/algorithms/circuit_mapper/sossa_mapper.py:222 _num_phase_gradient_qubitsreads the QROM state's ownrotation_bit_precisionsetting, which can diverge fromSOSSAMapper.rotation_bit_precision(used for SELECT). Since the walk shares a single phase-gradient register, the width should be derived from a single source of truth (e.g., the mapper setting) to avoid spurious mismatches.
python/src/qdk_chemistry/algorithms/qubit_mapper/sos.py:40- This mapper ignores the
MajoranaMappingpassed torun(), butQubitMapper’s documented contract is that encoding is determined by the provided mapping. Unconditionally warning and silently forcing Jordan–Wigner can produce surprising results for callers and noisy logs; it should instead validate that the requested mapping is supported (and matches the Hamiltonian’s mode count) and then proceed without warning.
python/tests/test_helpers.py:283 - These basis vectors are normalized but not orthogonal; calling them “orthogonal” is misleading for readers of the test helper.
examples/sossa_qre.ipynb:183
- This code cell line closes the AlgorithmRef call without the indentation needed inside the surrounding
if circuit_mapper is None:block, which will raise a SyntaxError when the notebook is executed.
")\n",
examples/sossa_qre.ipynb:191
- This code cell line closes the
builder = create(...)call without the indentation needed at the function scope, which will raise a SyntaxError when the notebook is executed.
")\n",
- Files reviewed: 32/32 changed files
- Comments generated: 0 new
- Review effort level: Lite
`_build_h2_dfthc_data` returned hand-written N=2, R=1, B=1, C=1 tensors, so the invariants these tests check -- that the sum-of-squares form is positive semidefinite, that it reproduces the physical Hamiltonian once E_SOS is added back, and that a walk eigenphase decodes to the ground-state energy -- were proved of an operator nothing ships. Reading examples/data at R=2, B=2, C=1 instead makes the shipped file's validity the thing under test, so a regenerated Hamiltonian that breaks a precondition fails here rather than silently producing a phase QPE cannot resolve. All four invariant tests pass against the current example, which locates the recent H2 histogram problem outside the Hamiltonian: H_gap has minimum eigenvalue +0.289, and the Hartree-Fock determinant is an exact eigenstate of it. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
There are API/runtime correctness issues in newly added SOS/Pauli-LCU container code paths that should be fixed before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
python/src/qdk_chemistry/data/qubit_operator/containers/pauli_lcu.py:143
PauliLCUContainerstorescoefficientswithout coercing it to a NumPy array. Several methods in this class (e.g.,to_json()usescoeffs.real/coeffs.imag, hashing uses_hash_array) assume annp.ndarray, so passing a Python list (or other sequence) will break at runtime even though the constructor type hints acceptnp.ndarrayand earlier behavior allowed array-like inputs.
- Files reviewed: 32/32 changed files
- Comments generated: 1
- Review effort level: Lite
| def _run_impl(self, hamiltonian: Hamiltonian, _mapping: MajoranaMapping) -> QubitOperator: | ||
| """Convert a factorized Hamiltonian to a structured SOS qubit operator.""" | ||
| if not isinstance(hamiltonian, Hamiltonian): | ||
| raise TypeError("SOSQubitMapper requires a Hamiltonian") | ||
|
|
||
| container = hamiltonian.get_container() | ||
| if not isinstance(container, FactorizedHamiltonianContainer): | ||
| raise TypeError("SOSQubitMapper requires a Hamiltonian backed by FactorizedHamiltonianContainer") | ||
|
|
||
| Logger.warn("SOSQubitMapper ignores the provided mapping and uses a single-mode Jordan-Wigner encoding") | ||
| return self._map_factorized_container(container, MajoranaMapping.jordan_wigner(1)) | ||
|
|
The bump is orthogonal to the SOSSA work: it touches only the dependency pins, the INSTALL.md extras table and the assertion that restates them, and it reviews cleanly on its own. It now lives in #710, based on main so it can land without waiting for this stack. Nothing here regresses in the meantime. The pin is a floor, so resolving \qdk>=1.30.0\ still installs 1.31 wherever it is available, which is what CI does; the floor only matters for an environment deliberately held at 1.30. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
SOSSA inner-PREPARE coefficient construction currently drops the sign of the SF identity-term coefficient, which breaks the phase/sign encoding needed for correct SOSSA generators.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
python/src/qdk_chemistry/algorithms/hamiltonian_unitary_builder/block_encoding/sossa.py:169
- The docstring says the inner conditional coefficients include the "absolute identity weight", but the Q# alias-sampling PREPARE uses coefficient sign to encode a phase. Documenting the identity term as absolute is misleading and suggests (incorrectly) that its sign is irrelevant.
- Files reviewed: 29/29 changed files
- Comments generated: 1
- Review effort level: Lite
| sf_rows = np.zeros((sf.shape[0], b_plus_1)) | ||
| if sf.size: | ||
| sf_rows[:, :-1] = sf[:, :-1].real | ||
| sf_rows[:, -1] = np.abs(sf[:, -1]) | ||
| return np.concatenate([delta, sf_rows], axis=0) |
| let majoranaStep : (Unit => Unit is Adj + Ctl) = () => { | ||
| MajoranaOp(isSF, dvsq, bEqBQubit, spin, sysRegDown[0]); | ||
| within { X(isSF); } apply { | ||
| Controlled Z([isSF], spinSF); |
There was a problem hiding this comment.
spinSF is in |+⟩ inside the reflection (H at :397, reflected at :402), and this Z fires exactly when isSF = 0 — so every D1/Q1 term picks up ⟨+|Z|+⟩ = 0 and the one-body Hamiltonian drops out of the circuit entirely. Driving the block encoding through the simulator on the H2 fixture, the measured block differs from the exact H_gap/Λ − I by 0.85, with only the two spin-free indices surviving and the one-body pair carrying 86% of the outer weight gone. Should the coherent Majorana sum sit on an inner register instead — and does MajoranaOp (:641-661) need fixing alongside, since it reuses one spin qubit for both the register swap and the X-vs-iY choice?
| // act on ALL sectors (0, 1, 2 excitations) so that its adjoint | ||
| // properly uncomputes after MajoranaOp changes the particle number. | ||
| for a in 0..N - 1 { | ||
| let angle = params.OneBodyRotationAngles[a][j]; |
There was a problem hiding this comment.
This sandwich is exp(iθ Y_j X_{j+1}), which rotates only the X-type Majoranas and mixes the |00⟩/|11⟩ sector, so it doesn't preserve particle number — I measured a violation of 0.972 against an operator norm of 1, producing |⟨1111|B|0000⟩| = 0.13 where the exact target is identically 0. There's also a transposition: within { chain } apply gives C† · Op · C, which uses row 0 of the single-particle matrix rather than column 0, so the rotated mode is the reflection of u. Is the comment at :499-502 describing an intended design here, or documenting this as a symptom?
| sf = np.asarray(sossa.two_body.coeffs) | ||
| sf_rows = np.zeros((sf.shape[0], b_plus_1)) | ||
| if sf.size: | ||
| sf_rows[:, :-1] = sf[:, :-1].real |
There was a problem hiding this comment.
The inner register is uncomputed before the reflection (SOSSAWalk.qs:394-400), so the block-encoded generator is Σ_b |β_b|² U_b — feeding raw w_b makes the spin-free generator quadratic in the weights instead of linear, and AliasSamplingStatePrep.qs:155 squares them too. Should these be sign-stripped √|w_b| normalised to the row one-norm that :160 already computes? Relatedly, where does sign(w_b) reach SELECT — both backends conjugate the phase away inside PREP† … PREP and MajoranaOp carries no per-b sign, though H2's all-positive, single-dominant weights hide both issues.
| # two-term LCU sqrt(|lambda|) * (X +/- iY)/2 on the single transformed spin orbital, sharing | ||
| # the generator's Givens rotation (length N-1). The +iY sign marks D1 and -iY marks Q1; the | ||
| # builder scales the one-norm by sqrt(2) for the two spin channels when forming outer coeffs. | ||
| pos_mask = eigenvalues > 0.0 |
There was a problem hiding this comment.
Exact-zero eigenvalues fall through both masks, so one_body gets fewer than N rows — but outer_prep_dim (block_encoding/sossa.py:141) and _compute_free_rider_data (:254) still reserve N one-body slots, so every spin-free amplitude decodes as Q1 with the wrong rank. SOSSAWalk.TestSelectDQ reproduces it as QSharpError: index out of range: 1, and it's reachable from the public constructor — a pure two-body payload gives eigenvalues of exactly 0.0, with validation checking dimensions but not rank (and no shipped fixture is rank-deficient, so the tests can't catch it). Would you rather pad the dropped modes with √λ = 0 generators, or derive x_o from the actual one-body row count?
| # the generator's Givens rotation (length N-1). The +iY sign marks D1 and -iY marks Q1; the | ||
| # builder scales the one-norm by sqrt(2) for the two spin channels when forming outer coeffs. | ||
| pos_mask = eigenvalues > 0.0 | ||
| neg_mask = eigenvalues < 0.0 |
There was a problem hiding this comment.
Beyond the exact-zero case above, would a screening threshold help here — eigensolver noise at ~1e-17 passes > 0.0 and becomes a real generator riding an essentially arbitrary nullspace eigenvector. On an N=54, rank-10 probe that produced 44 spurious generators and 2,332 meaningless Givens angles. The package already defaults to 1e-12 in qdk_qubit_mapper.py:39-40, though any fix still has to preserve the N outer slots, so it couples to the layout question.
| n_coeffs = inner_coeffs.shape[1] | ||
| n_index_bits = math.ceil(math.log2(n_coeffs)) if n_coeffs > 1 else 1 | ||
|
|
||
| if algorithm == "controlled_alias_sampling": |
There was a problem hiding this comment.
This densifies a 24-qubit dump — 16.7M amplitudes, ~256 MiB of numpy plus ~128 MiB of list pointers — and post-processes it in a Python loop I measured at 14.873 s. :165-173 does 25 qubits (~512 MiB) and :436-443 does 25 qubits twice, scaling cleanly as 2^n from 0.506 s at 20 qubits. None of the three are marked @pytest.mark.slow, so they all land in the default CI run — is that intentional?
| # ═══════════════════════════════════════════════════════════════════════════════ | ||
|
|
||
|
|
||
| class TestSOSSAWalkLogicalCounts: |
There was a problem hiding this comment.
This docstring says the class verifies Toffoli scaling and that walk power multiplies Toffolis linearly, but the only test (:506-542) asserts a qubit-count bound. There's no Toffoli assertion, and power is never varied. Trim the docstring, or is there a missing test?
| __all__ = ["FactorizedHamiltonianMetadata", "RotatedPaulis", "SOSContainer"] | ||
|
|
||
|
|
||
| def _complex_block_to_json(coeffs: np.ndarray) -> dict[str, Any]: |
There was a problem hiding this comment.
pauli_lcu.py:465-469 writes {real, imag, dtype} and restores the dtype on load at :518-521, while this one writes {real, imag} and hardcodes complex. Harmless in practice since SOS coefficients are always complex128, but it's a near-duplicate helper with a different wire format inside the same package. Reuse the existing one, or match its shape?
| eprint = {2011.03494}, | ||
| archivePrefix = {arXiv} | ||
| } | ||
| @article{Berry2024, |
There was a problem hiding this comment.
Berry2024 is the only bibliography change in this PR, but seems like nothing cites it.
| ) | ||
| one_body_coeffs = 0.5 * np.stack([sqrt_lambdas, 1j * signs * sqrt_lambdas], axis=1) | ||
|
|
||
| # Spin-free two-body generators: one rotated-Z per (rank, basis), rotations shared across |
There was a problem hiding this comment.
This comment writes the identity coefficient as W^{rc} I, but the code puts identity_weights there — the paper's w_B^{(rc)} — while the local w0 at :101 is the actual W^{(rc)}. The two symbols are swapped in the one place where the distinction decides a sign. Renaming w0 to something like w_rc and correcting the comment would help, especially given the np.abs question on that same weight.
There was a problem hiding this comment.
🟡 Changes recommended
There are at least two correctness issues that can break SOSSA invariants (generator count vs N, and a Q# inner-PREPARE bit-width edge case) that should be fixed before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
python/src/qdk_chemistry/utils/qsharp/src/SOSSAWalk.qs:718
- MakeInnerPrepareAliasSampling computes nIndexBits as BitSizeI(nCoeffs - 1) without guarding nCoeffs<=1. If B=0 (so B+1=1) or a degenerate coefficient row is passed, this can yield 0 index bits and make slices like innerReg[0..nIndexBits - 1] invalid. The direct inner-prepare path already uses the guarded BitSizeI((if nCoeffs > 1 { nCoeffs } else { 2 }) - 1) pattern; the alias-sampling path should match it.
- Files reviewed: 29/29 changed files
- Comments generated: 1
- Review effort level: Lite
| pos_mask = eigenvalues > 0.0 | ||
| neg_mask = eigenvalues < 0.0 | ||
| one_body_vectors = np.concatenate([eigenvectors[:, pos_mask].T, eigenvectors[:, neg_mask].T], axis=0) | ||
| sqrt_lambdas = np.sqrt(np.concatenate([eigenvalues[pos_mask], -eigenvalues[neg_mask]])) | ||
| signs = np.concatenate([np.ones(int(pos_mask.sum())), -np.ones(int(neg_mask.sum()))]) | ||
| num_positive = int(pos_mask.sum()) |
e0867d6 to
e998897
Compare
There was a problem hiding this comment.
🟡 Changes recommended
There are verified correctness issues in the SOS/SOSSA coefficient construction that can alter the encoded operator (identity-term handling and sign preservation).
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
python/src/qdk_chemistry/algorithms/hamiltonian_unitary_builder/block_encoding/sossa.py:179
- The inner-PREPARE coefficient table keeps signed coefficients so
ConditionalAliasSamplingPrepareWithFreeRidercan apply the sign as a phase (see Q# doc comment inSOSSAWalk.qs). Takingnp.absof the identity-column (b == B) silently drops the sign of that coefficient, which changes the SF generator (the identity term participates in cross-terms inside the square).
Keep the signed identity coefficient instead of its magnitude.
sf = np.asarray(sossa.two_body.coeffs)
sf_rows = np.zeros((sf.shape[0], b_plus_1))
if sf.size:
sf_rows[:, :-1] = sf[:, :-1].real
sf_rows[:, -1] = np.abs(sf[:, -1])
- Files reviewed: 29/29 changed files
- Comments generated: 1
- Review effort level: Lite
| sf_basis = np.transpose(weights_rbc, (0, 2, 1)).reshape(num_ranks * num_copies, num_bases) * sf_coefficient | ||
| two_body_coeffs = np.concatenate([sf_basis, identity_weights.reshape(-1, 1)], axis=1).astype(complex) | ||
|
|
||
| negative_sum = float(-np.sum(eigenvalues[neg_mask])) | ||
| w0 = identity_weights - weights_rbc.sum(axis=1) |
Implements Sum-of-Squares Spectral Amplification (SOSSA) following Low et al. 2025, PRX 15, with the qubitized walk and unary-iteration machinery it needs.
Stacked on #621
This PR is the top layer of a native GitHub stack (#686):
Alias sampling was merged into this branch and this PR's base retargeted onto #621, so
the diff here is now the SOSSA-only remainder: 44 files / +6,887, down from 70 files
/ +9,639 against
main. Review #621 first; GitHub will cascade the merge.What this PR adds
SOSSA block encoding and walk (
SOSSAWalk.qs), the double-factorized Hamiltonian unitary builder, the SOSSA controlled circuit mapper, and resource estimation support. The alias-sampling / QROM / SelectSwap state preparation this builds on is #621, the layer below.Data flow
%%{init: {"flowchart": {"padding": 30, "nodeSpacing": 60, "rankSpacing": 55}}}%% flowchart TD subgraph S1["Stage 1 — qubit mapping"] HAM["Hamiltonian ⟨FactorizedHamiltonianContainer⟩<br/>U : R*B*N W : R*B*C WB : R,C<br/>signs : R, h1, E_core, E_gap"] HAM --> QM[" SOSSAQubitMapper "] QM --> SOSC["QubitOperator ⟨SOSSAContainer⟩<br/>one_body : RotatedPaulis angles, coeffs, X/Y<br/>two_body : RotatedPaulis angles, coeffs, Z<br/>num_positive_one_body_terms, energy_shift"] end subgraph S2["Stage 2 — unitary builder"] SOSC --> SB[" SOSSABuilder "] SB --> WALK["UnitaryRepresentation ⟨SOSSAWalkContainer⟩<br/>outer_prepare : Wavefunction ∝ c<br/>outer_prepare_probabilities ∝ c²<br/>SOSSAInnerPrepare : cond. coeffs + free-rider bits<br/>SOSSASelect : Givens angles<br/>Λ = ½Σc², power, energy_shift"] end subgraph S3["Stage 3 — circuit mapper"] WALK --> CM[" SOSSAMapper "] CM --> CIRC["Circuit<br/>MakeControlledSOSSAWalkOp / …Circuit"] end classDef data fill:#dbeafe,stroke:#2563eb,color:#0b1b34 classDef algo fill:#fef3c7,stroke:#d97706,color:#3b2a06 class HAM,SOSC,WALK,CIRC data class QM,SB,CM algoTesting
End-to-end energy recovery is asserted for H2 in both the iterative and unary QPE paths. Both use the quadratically suppressed tolerance
Λ(1 − cos(2π/N))rather than the linearΛ·2π/N— SOSSA's ground state sits at φ ≈ 1/2 wheredE/dφ = 0, so the linear bound is roughly 15× too loose to be meaningful. Each test also asserts that the amplified bound is at least 5× tighter than the linear one, so the distinction can't silently regress.