2nd order Trotter and Corresponding Error Bounds#384
Open
v-agamshayit wants to merge 25 commits intomainfrom
Open
2nd order Trotter and Corresponding Error Bounds#384v-agamshayit wants to merge 25 commits intomainfrom
v-agamshayit wants to merge 25 commits intomainfrom
Conversation
v-agamshayit
requested review from
ConradJohnston,
nabbelbabbel and
wavefunction91
as code owners
Contributor
There was a problem hiding this comment.
Pull request overview
Adds second-order (Strang) Trotter time evolution support and extends the Trotter step-count estimation utilities with second-order naive and commutator-based error bounds, along with new tests covering the added behavior.
Changes:
- Extend
Trotterbuilder to supportorder=2decomposition. - Add second-order commutator-bound calculation and integrate it into step estimation.
- Expand Python test coverage for second-order decomposition and error-bound step selection.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| python/src/qdk_chemistry/algorithms/time_evolution/builder/trotter.py | Add order=2 path in builder and implement second-order step decomposition. |
| python/src/qdk_chemistry/algorithms/time_evolution/builder/trotter_error.py | Extend naive/commutator step estimators to support order=2. |
| python/src/qdk_chemistry/utils/pauli_commutation.py | Add nested-commutator vanishing check and second-order commutator bound computation. |
| python/tests/test_trotter_error.py | Add second-order error-bound tests and update unsupported-order checks. |
| python/tests/test_time_evolution_trotter.py | Add second-order builder tests and accuracy-aware step selection tests for order=2. |
Comments suppressed due to low confidence (14)
python/src/qdk_chemistry/algorithms/time_evolution/builder/trotter.py:223
- The docstring lists an
orderargument here, but_decompose_trotter_stepdoesn’t accept anorderparameter (it reads fromself._settings). This is misleading for readers and for generated docs; please remove or reword this Args entry.
"""Decompose a single Trotter step into exponentiated Pauli terms.
Args:
qubit_hamiltonian: The qubit Hamiltonian to be decomposed.
time: The evolution time for the single step.
order: The order of the Trotter decomposition.
atol: Absolute tolerance for filtering small coefficients.
python/tests/test_trotter_error.py:138
- This test compares
order=2commutator steps against the default naive steps (which areorder=1). If the intent is “commutator bound is never looser than naive” for second-order, calltrotter_steps_naive(..., order=2)so both bounds are for the same order.
def test_tighter_than_naive_second_order(self):
"""Test that commutator bound is never looser than naive."""
h = QubitHamiltonian(pauli_strings=["X", "Z"], coefficients=[1.0, 1.0])
eps = 0.01
time = 1.0
n_naive = trotter_steps_naive(h, time, eps)
n_comm = trotter_steps_commutator(h, time, eps, order=2)
assert n_comm <= n_naive
python/tests/test_trotter_error.py:122
- The arithmetic in this comment is inconsistent:
ceil(10) = 4is incorrect and the intermediate value here isn’t 10 for the given formula. Please fix the comment so it matches the actual calculation that leads to 4 steps (to avoid misleading future maintainers).
# X and Z anticommute: commutator bound = 2 * 2**2 = 8
# N = ceil(sqrt(8) * 1**(3/2) / (sqrt(3! * 0.1))) = ceil(10) = 4
h = QubitHamiltonian(pauli_strings=["X", "Z"], coefficients=[1.0, 1.0])
python/tests/test_time_evolution_trotter.py:395
- The explanatory comment doesn’t match the asserted result. With the current
commutator_bound_second_orderimplementation forH=X+Z, the bound is 8 (not 4), which yieldsN=12fort=1, eps=0.01. Please update the comment so the derivation matches the expected step count.
# H = X + Z, X and Z anticommute -> commutator bound = 4
# N = ceil(sqrt(4) * t^(3/2) / (sqrt(3! * eps))) = 17
hamiltonian = QubitHamiltonian(pauli_strings=["X", "Z"], coefficients=[1.0, 1.0])
builder = Trotter(target_accuracy=0.01, order=2)
unitary = builder.run(hamiltonian, time=1.0)
container = unitary.get_container()
assert container.step_reps == 12
python/tests/test_time_evolution_trotter.py:404
- This comment uses
2^1/2, but the implemented naive formula fororder=2scales likeone_norm^(3/2) / sqrt(eps)(fort=1). Consider updating the comment to2^(3/2) / 0.1 = 28.28 -> 29so it matches the actual computation.
container = unitary.get_container()
# one_norm = 2, N = ceil(2^1/2 / 0.01) = 29
assert container.step_reps == 29
python/src/qdk_chemistry/algorithms/time_evolution/builder/trotter_error.py:110
- The displayed second-order equation (
N_2 = t^3/3! * sum ...) does not match what this function returns (a step count) nor the scaling implemented below (which solves forNgivenε, including a factorial factor and a root). Please rewrite the math block so it definesNfororder=2in terms ofεand the computed commutator sum.
The tighter commutator-based bound from Childs *et al.* (2021) is
.. math::
N_1 = \left\lceil \frac{t^2}{2\epsilon}
\sum_{j<k} \lVert [\alpha_j P_j,\, \alpha_k P_k] \rVert
\right\rceil
N_2 = \frac{t^3}{3!} \sum_{j < k < l}
\lVert \lvert [\alpha_j P_l,\, [\alpha_j P_j,\, \alpha_k P_k] \rVert ] \rVert
python/src/qdk_chemistry/utils/pauli_commutation.py:321
- The math in this docstring appears inconsistent (index names don’t match, e.g.
P_i/P_l, and the triple-sum conditionj < k < ldoesn’t reflect the(i, j, k)iteration used below). Please update the formula/description so it matches the actual bound being computed.
r"""Compute the second-order Trotter commutator bound.
For a Hamiltonian :math:`H = \sum_j \alpha_j P_j` the second-order
(Lie-Trotter) product formula has error bounded by
.. math::
\lVert U(t) - S_2(t) \rVert \le
\frac{t^3}{3!} \sum_{j < k < l}
\lVert \lvert [\alpha_j P_i,\, [\alpha_j P_j,\, \alpha_k P_k] \rVert ] \rVert
python/src/qdk_chemistry/utils/pauli_commutation.py:331
- This docstring says users should multiply by
t^3 / (3! * N)to get the per-step error, but the second-order Strang error scales likeO(t^3 / N^2)for a full evolution split intoNsteps. Please correct theN-dependence described here to match howtrotter_steps_commutator(..., order=2)solves forN.
This function returns
:math:`\sum_{j < k < l}
\lVert \lvert [\alpha_j P_i,\, [\alpha_j P_j,\, \alpha_k P_k] \rVert ] \rVert`,
so the user can multiply by :math:`t^{3} / (3! * N)` to get the per-step
error.
python/tests/test_time_evolution_trotter.py:284
- The comment says the second-order error should scale as
O(t^2), but the assertion usesatol=t**3, which corresponds to the expectedO(t^3)global error scaling for a second-order (Strang) formula at fixed step count. Please correct the comment to avoid confusion.
# Compare: the error should scale as O(t^2) for second-order Trotter
assert np.allclose(u_trot, u_exact, atol=t**3)
python/src/qdk_chemistry/algorithms/time_evolution/builder/trotter_error.py:87
- This return statement is likely to exceed the configured Ruff line length (120) and is hard to read/maintain in its current form. Consider breaking the expression into intermediate variables or multiple lines so it passes formatting/linting consistently.
real_terms = hamiltonian.get_real_coefficients(tolerance=weight_threshold)
one_norm = sum(abs(coeff) for _, coeff in real_terms)
return max(1, math.ceil((one_norm ** (1 + 1 / order) * time ** (1 + 1 / order)) / (target_accuracy ** (1 / order))))
python/src/qdk_chemistry/algorithms/time_evolution/builder/trotter.py:152
- This method is now used for both order=1 and order=2, but the docstring still describes only the first-order formula. Please update the docstring to document the second-order (Strang) construction as well, since it’s part of the public behavior now.
def _first_or_second_order_trotter(self, qubit_hamiltonian: QubitHamiltonian, time: float) -> TimeEvolutionUnitary:
r"""Construct the time evolution unitary using first-order Trotter decomposition.
The First Order Trotter method approximates the time evolution operator :math:`e^{-iHt}`
by decomposing the Hamiltonian H into a sum of terms and using the product formula:
:math:`e^{-iHt} \approx \left[\prod_i e^{-iH_i t/n}\right]^n`, where n is the number of divisions.
python/src/qdk_chemistry/algorithms/time_evolution/builder/trotter.py:255
get_real_coefficients(tolerance=atol)is called three times in theorder == 2branch (slice, last element, reversed slice). Cache the filtered term list once to avoid repeated work and to guarantee consistent ordering across the forward/center/reverse parts.
# \prod_{i=1}^{L-1} e^{-iH_i t/(2n)}
for label, coeff in qubit_hamiltonian.get_real_coefficients(tolerance=atol)[:-1]:
mapping = self._pauli_label_to_map(label)
angle = coeff * time / 2
terms.append(ExponentiatedPauliTerm(pauli_term=mapping, angle=angle))
# e^{-iH_L t/n}
label, coeff = qubit_hamiltonian.get_real_coefficients(tolerance=atol)[-1]
mapping = self._pauli_label_to_map(label)
angle = coeff * time
terms.append(ExponentiatedPauliTerm(pauli_term=mapping, angle=angle))
# \prod_{i=L-1}^1 e^{-iH_i t/(2n)}
for label, coeff in reversed(qubit_hamiltonian.get_real_coefficients(tolerance=atol)[:-1]):
mapping = self._pauli_label_to_map(label)
python/src/qdk_chemistry/utils/pauli_commutation.py:43
commutator_bound_second_order()is a new top-level API in this module but isn’t included in__all__, whilecommutator_bound_first_order()is. If the second-order bound is intended to be public (it’s already imported elsewhere), add it to__all__for consistency.
__all__: list[str] = [
"commutator_bound_first_order",
"do_pauli_labels_commute",
"do_pauli_labels_qw_commute",
"do_pauli_maps_commute",
"do_pauli_maps_qw_commute",
"does_nested_commutator_vanish",
"get_commutation_checker",
]
python/tests/test_trotter_error.py:151
- The docstring says
t^3scaling, but the assertion below checkst^(3/2)scaling (2**1.5). Please update the docstring/comment so it matches the expected scaling for howNdepends ontfor the second-order bound.
def test_time_scaling_second_order(self):
"""Test that step count scales with t^3."""
h = QubitHamiltonian(pauli_strings=["X", "Z"], coefficients=[1.0, 1.0])
n1 = trotter_steps_commutator(h, 1.0, 0.1, order=2)
n2 = trotter_steps_commutator(h, 2.0, 0.1, order=2)
# n2 should be approximately 2**1.5 * n1 (t^1.5 scaling)
assert n2 >= math.ceil(2**1.5) * n1 - 1 # Allow for ceiling effects
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
python/src/qdk_chemistry/algorithms/time_evolution/builder/trotter_error.py
Outdated
Show resolved
Hide resolved
python/src/qdk_chemistry/algorithms/time_evolution/builder/trotter.py
Outdated
Show resolved
Hide resolved
python/src/qdk_chemistry/algorithms/time_evolution/builder/trotter_error.py
Outdated
Show resolved
Hide resolved
Contributor
|
@v-agamshayit I've opened a new pull request, #385, to work on those changes. Once the pull request is ready, I'll request review from you. |
…d orders Co-authored-by: v-agamshayit <257422224+v-agamshayit@users.noreply.github.com>
4 tasks
Contributor
|
@v-agamshayit I've opened a new pull request, #386, to work on those changes. Once the pull request is ready, I'll request review from you. |
📊 Coverage Summary
Detailed Coverage ReportsC++ Coverage DetailsPython Coverage DetailsPybind11 Coverage Details |
…upported values (#385)
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (11)
python/src/qdk_chemistry/utils/pauli_commutation.py:331
- The second-order bound docstring/math appears inconsistent with the actual implementation: the code sums over i, j, k with j<k (including repeated indices like i=j), but the docstring states a j<k<l triple sum and has mismatched indices/brackets. Please update the formula/notation to match what is actually computed (and fix the malformed \lVert/\rVert expression) so users don’t misinterpret the bound.
r"""Compute the second-order Trotter commutator bound.
For a Hamiltonian :math:`H = \sum_j \alpha_j P_j` the second-order
(Lie-Trotter) product formula has error bounded by
.. math::
\lVert U(t) - S_2(t) \rVert \le
\frac{t^3}{3!} \sum_{j < k < l}
\lVert \lvert [\alpha_j P_i,\, [\alpha_j P_j,\, \alpha_k P_k] \rVert ] \rVert
For Pauli strings the spectral norm of the commutator is
* 0 if :math:`P_j` and :math:`P_k` commute, or
* :math:`2 |\alpha_j| |\alpha_k|` if they anticommute.
This function returns
:math:`\sum_{j < k < l}
\lVert \lvert [\alpha_j P_i,\, [\alpha_j P_j,\, \alpha_k P_k] \rVert ] \rVert`,
so the user can multiply by :math:`t^{3} / (3! * N)` to get the per-step
error.
python/src/qdk_chemistry/algorithms/time_evolution/builder/trotter_error.py:88
- The naive-bound docstring still states the first-order formula
N = ceil((sum|α|)^2 t^2 / ε), but the implementation now uses an order-dependent exponent (and supportsorder=2). Update the docstring (and module-level description if needed) to document the order-2 formula used here, otherwise the docs disagree with runtime behavior.
r"""Compute the number of Trotter steps using the naive bound.
The naive (triangle-inequality) bound is
.. math::
N = \left\lceil \frac{(\sum_j |\alpha_j|)^2 \, t^2}{\epsilon} \right\rceil
where :math:`\sum_j |\alpha_j|` is the 1-norm of the Hamiltonian
coefficients.
Args:
hamiltonian: The qubit Hamiltonian to simulate.
time: The total evolution time *t*.
target_accuracy: The target accuracy :math:`\epsilon > 0`.
order: The order of the Trotter-Suzuki product formula.
weight_threshold: Absolute threshold below which coefficients are discarded.
Returns:
The minimum number of Trotter steps (at least 1).
Raises:
ValueError: If ``target_accuracy`` is not positive.
NotImplementedError: If *order* is not yet supported.
"""
if target_accuracy <= 0:
raise ValueError(f"target_accuracy must be positive, got {target_accuracy}.")
if order != 1 and order != 2:
raise NotImplementedError(
f"Trotter step estimation for order {order} is not yet implemented. "
"Only orders 1 or 2 are currently supported."
)
real_terms = hamiltonian.get_real_coefficients(tolerance=weight_threshold)
one_norm = sum(abs(coeff) for _, coeff in real_terms)
return max(1, math.ceil((one_norm ** (1 + 1 / order) * time ** (1 + 1 / order)) / (target_accuracy ** (1 / order))))
python/src/qdk_chemistry/algorithms/time_evolution/builder/trotter.py:153
- This method (and its docstring) still describes first-order Trotter only, but it is now used for both
order=1andorder=2. Please update the docstring to describe both decompositions (or make the wording order-agnostic) so that public behavior matches documentation.
def _first_or_second_order_trotter(self, qubit_hamiltonian: QubitHamiltonian, time: float) -> TimeEvolutionUnitary:
r"""Construct the time evolution unitary using first-order Trotter decomposition.
The First Order Trotter method approximates the time evolution operator :math:`e^{-iHt}`
by decomposing the Hamiltonian H into a sum of terms and using the product formula:
:math:`e^{-iHt} \approx \left[\prod_i e^{-iH_i t/n}\right]^n`, where n is the number of divisions.
python/src/qdk_chemistry/algorithms/time_evolution/builder/trotter.py:223
_decompose_trotter_step’s docstring lists anorderargument, but the function signature doesn’t accept it (it readsorderfrom settings instead). Please remove that parameter from the docstring or add it to the signature to avoid misleading API docs.
def _decompose_trotter_step(
self, qubit_hamiltonian: QubitHamiltonian, time: float, *, atol: float = 1e-12
) -> list[ExponentiatedPauliTerm]:
"""Decompose a single Trotter step into exponentiated Pauli terms.
Args:
qubit_hamiltonian: The qubit Hamiltonian to be decomposed.
time: The evolution time for the single step.
order: The order of the Trotter decomposition.
atol: Absolute tolerance for filtering small coefficients.
python/tests/test_trotter_error.py:138
- In the second-order version of this test,
n_naiveis computed with the defaultorder=1, whilen_commis computed withorder=2. That makes the assertionn_comm <= n_naivecompare different orders and doesn’t actually test “commutator bound is never looser than naive” for second-order. Passorder=2totrotter_steps_naivehere.
def test_tighter_than_naive_second_order(self):
"""Test that commutator bound is never looser than naive."""
h = QubitHamiltonian(pauli_strings=["X", "Z"], coefficients=[1.0, 1.0])
eps = 0.01
time = 1.0
n_naive = trotter_steps_naive(h, time, eps)
n_comm = trotter_steps_commutator(h, time, eps, order=2)
assert n_comm <= n_naive
python/src/qdk_chemistry/utils/pauli_commutation.py:43
commutator_bound_second_order()is a top-level utility that is imported and used elsewhere, but it is not included in this module’s__all__(whilecommutator_bound_first_orderis). Either add it to__all__for consistency/public API clarity, or rename it with a leading underscore if it’s intended to stay private.
__all__: list[str] = [
"commutator_bound_first_order",
"do_pauli_labels_commute",
"do_pauli_labels_qw_commute",
"do_pauli_maps_commute",
"do_pauli_maps_qw_commute",
"does_nested_commutator_vanish",
"get_commutation_checker",
]
python/src/qdk_chemistry/algorithms/time_evolution/builder/trotter_error.py:111
- The commutator-bound docstring’s second-order section is internally inconsistent (e.g.,
N_2is not a step-count expression/ceil, indices don’t match, and the norm expression appears malformed). Since this function now supportsorder=2with a specific closed-form step-count formula, please rewrite the math block to match the implemented calculation for orders 1 and 2.
r"""Compute the number of Trotter steps using the commutator bound.
The tighter commutator-based bound from Childs *et al.* (2021) is
.. math::
N_1 = \left\lceil \frac{t^2}{2\epsilon}
\sum_{j<k} \lVert [\alpha_j P_j,\, \alpha_k P_k] \rVert
\right\rceil
N_2 = \frac{t^3}{3!} \sum_{j < k < l}
\lVert \lvert [\alpha_j P_l,\, [\alpha_j P_j,\, \alpha_k P_k] \rVert ] \rVert
For Pauli strings the commutator norm is :math:`2|\alpha_j||\alpha_k|`
python/tests/test_trotter_error.py:123
- The comment math for the second-order commutator example is incorrect:
sqrt(8) / sqrt(3! * 0.1)is about 3.65 (not 10), soceil(...) = 4but the intermediate value in the comment should be fixed to avoid confusion when maintaining these tests.
# X and Z anticommute: commutator bound = 2 * 2**2 = 8
# N = ceil(sqrt(8) * 1**(3/2) / (sqrt(3! * 0.1))) = ceil(10) = 4
h = QubitHamiltonian(pauli_strings=["X", "Z"], coefficients=[1.0, 1.0])
assert trotter_steps_commutator(h, 1.0, 0.1, order=2) == 4
python/tests/test_trotter_error.py:151
- This test’s docstring says the step count “scales with t^3”, but the asserted scaling is
t^(3/2)(sinceN ~ t^(1 + 1/order)fororder=2). Please update the docstring/comment to match the actual scaling being tested.
def test_time_scaling_second_order(self):
"""Test that step count scales with t^3."""
h = QubitHamiltonian(pauli_strings=["X", "Z"], coefficients=[1.0, 1.0])
n1 = trotter_steps_commutator(h, 1.0, 0.1, order=2)
n2 = trotter_steps_commutator(h, 2.0, 0.1, order=2)
# n2 should be approximately 2**1.5 * n1 (t^1.5 scaling)
assert n2 >= math.ceil(2**1.5) * n1 - 1 # Allow for ceiling effects
python/tests/test_time_evolution_trotter.py:284
- The comment says “error should scale as O(t^2) for second-order Trotter”, but the assertion uses
atol=t**3, which corresponds to third-order error scaling for a second-order (Strang) formula. Please correct the comment so it matches the actual expected scaling being enforced.
# Compare: the error should scale as O(t^2) for second-order Trotter
assert np.allclose(u_trot, u_exact, atol=t**3)
python/tests/test_time_evolution_trotter.py:404
- The inline derivation comments for the second-order commutator/naive step counts are inconsistent with the actual formulas and expected assertions (e.g., commutator bound for X+Z under the current implementation is 8, and the resulting N is 12; the naive bound comment also uses the wrong exponent/denominator). Please fix these comments to reflect the same calculation the code is testing, otherwise future updates will be error-prone.
def test_target_accuracy_commutator_bound_second_order(self):
"""Test that target_accuracy with commutator bound computes correct step count."""
# H = X + Z, X and Z anticommute -> commutator bound = 4
# N = ceil(sqrt(4) * t^(3/2) / (sqrt(3! * eps))) = 17
hamiltonian = QubitHamiltonian(pauli_strings=["X", "Z"], coefficients=[1.0, 1.0])
builder = Trotter(target_accuracy=0.01, order=2)
unitary = builder.run(hamiltonian, time=1.0)
container = unitary.get_container()
assert container.step_reps == 12
def test_target_accuracy_naive_bound_second_order(self):
"""Test that target_accuracy with naive bound computes correct step count."""
hamiltonian = QubitHamiltonian(pauli_strings=["X", "Z"], coefficients=[1.0, 1.0])
builder = Trotter(target_accuracy=0.01, error_bound="naive", order=2)
unitary = builder.run(hamiltonian, time=1.0)
container = unitary.get_container()
# one_norm = 2, N = ceil(2^1/2 / 0.01) = 29
assert container.step_reps == 29
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (11)
python/src/qdk_chemistry/utils/pauli_commutation.py:331
- The second-order commutator-bound docstring has several math/notation issues (e.g., mixed indices
P_ivsP_j, mismatched brackets, andt^3/(3!*N)which suggests1/Nscaling). Since the step estimator uses1/N^2scaling for order=2, update this docstring so the bound expression and how to apply it (including theN^2dependence) are correct and unambiguous.
r"""Compute the second-order Trotter commutator bound.
For a Hamiltonian :math:`H = \sum_j \alpha_j P_j` the second-order
(Lie-Trotter) product formula has error bounded by
.. math::
\lVert U(t) - S_2(t) \rVert \le
\frac{t^3}{3!} \sum_{j < k < l}
\lVert \lvert [\alpha_j P_i,\, [\alpha_j P_j,\, \alpha_k P_k] \rVert ] \rVert
For Pauli strings the spectral norm of the commutator is
* 0 if :math:`P_j` and :math:`P_k` commute, or
* :math:`2 |\alpha_j| |\alpha_k|` if they anticommute.
This function returns
:math:`\sum_{j < k < l}
\lVert \lvert [\alpha_j P_i,\, [\alpha_j P_j,\, \alpha_k P_k] \rVert ] \rVert`,
so the user can multiply by :math:`t^{3} / (3! * N)` to get the per-step
error.
python/src/qdk_chemistry/algorithms/time_evolution/builder/trotter.py:153
- This method’s docstring still describes first-order Trotter decomposition only, but it is now used for both order 1 and order 2. Update the docstring to reflect second-order (Strang) splitting behavior, or split into two clearly documented methods.
def _first_or_second_order_trotter(self, qubit_hamiltonian: QubitHamiltonian, time: float) -> TimeEvolutionUnitary:
r"""Construct the time evolution unitary using first-order Trotter decomposition.
The First Order Trotter method approximates the time evolution operator :math:`e^{-iHt}`
by decomposing the Hamiltonian H into a sum of terms and using the product formula:
:math:`e^{-iHt} \approx \left[\prod_i e^{-iH_i t/n}\right]^n`, where n is the number of divisions.
python/src/qdk_chemistry/algorithms/time_evolution/builder/trotter.py:223
- The
_decompose_trotter_stepdocstring lists anorderargument, but the function signature does not acceptorder(it is read from settings instead). Please remove/adjust this docstring entry to match the actual API.
def _decompose_trotter_step(
self, qubit_hamiltonian: QubitHamiltonian, time: float, *, atol: float = 1e-12
) -> list[ExponentiatedPauliTerm]:
"""Decompose a single Trotter step into exponentiated Pauli terms.
Args:
qubit_hamiltonian: The qubit Hamiltonian to be decomposed.
time: The evolution time for the single step.
order: The order of the Trotter decomposition.
atol: Absolute tolerance for filtering small coefficients.
python/src/qdk_chemistry/algorithms/time_evolution/builder/trotter.py:145
_run_implduplicates the same call fororder == 1andorder == 2. This can be simplified to a single membership check (e.g.,if order in {1, 2}) to avoid drift if behavior changes in the future.
if self._settings.get("order") == 1:
return self._first_or_second_order_trotter(qubit_hamiltonian, time)
if self._settings.get("order") == 2:
return self._first_or_second_order_trotter(qubit_hamiltonian, time)
raise NotImplementedError("Only orders 1 or 2 are currently supported.")
python/tests/test_trotter_error.py:144
- The test says “step count scales with t^3”, but the assertion/comment below uses
t^1.5scaling (which is what you’d expect whenN ~ t^(3/2)from anO(t^3/N^2)error). Update the docstring/comment so they’re consistent with the actual scaling being tested.
def test_time_scaling_second_order(self):
"""Test that step count scales with t^3."""
h = QubitHamiltonian(pauli_strings=["X", "Z"], coefficients=[1.0, 1.0])
n1 = trotter_steps_commutator(h, 1.0, 0.1, order=2)
n2 = trotter_steps_commutator(h, 2.0, 0.1, order=2)
# n2 should be approximately 2**1.5 * n1 (t^1.5 scaling)
assert n2 >= math.ceil(2**1.5) * n1 - 1 # Allow for ceiling effects
python/src/qdk_chemistry/utils/pauli_commutation.py:43
commutator_bound_second_orderis now part of the module’s public surface (imported by other code/tests) but it isn’t exported in__all__, unlikecommutator_bound_first_order. Consider adding it to__all__for consistency.
__all__: list[str] = [
"commutator_bound_first_order",
"do_pauli_labels_commute",
"do_pauli_labels_qw_commute",
"do_pauli_maps_commute",
"do_pauli_maps_qw_commute",
"does_nested_commutator_vanish",
"get_commutation_checker",
]
python/src/qdk_chemistry/utils/pauli_commutation.py:352
- This nested-loop structure iterates
kfromi+1independently ofj, so it includes cases likek == jand counts both(j,k)and(k,j). If the intent is a redundancy-free sum over distinct triples (as the docstring says), the loop bounds should be tightened (e.g.,i < j < k). If the redundancy is intentional for the bound, the docstring should be updated to describe the exact summation being computed.
for i in range(n):
for j in range(i + 1, n):
for k in range(i + 1, n):
if not does_nested_commutator_vanish(pauli_labels[k], pauli_labels[j], pauli_labels[i]):
total_term1 += 2.0**2 * abs(coefficients[i]) * abs(coefficients[j]) * abs(coefficients[k])
python/tests/test_time_evolution_trotter.py:403
- The explanatory comments compute a commutator bound / expected
Nthat doesn’t match the assertedstep_reps(comment says bound=4 and N=17, assertion expects 9). Please fix the comment so it reflects the actual bound and step-count formula being used.
def test_target_accuracy_commutator_bound_second_order(self):
"""Test that target_accuracy with commutator bound computes correct step count."""
# H = X + Z, X and Z anticommute -> commutator bound = 4
# N = ceil(sqrt(4) * t^(3/2) / (sqrt(3! * eps))) = 17
hamiltonian = QubitHamiltonian(pauli_strings=["X", "Z"], coefficients=[1.0, 1.0])
builder = Trotter(target_accuracy=0.01, order=2)
unitary = builder.run(hamiltonian, time=1.0)
container = unitary.get_container()
assert container.step_reps == 9
python/tests/test_time_evolution_trotter.py:412
- This comment has a math typo: for second-order naive scaling you expect
N ~ one_norm^(3/2) * t^(3/2) / sqrt(eps), but the comment currently saysceil(2^1/2 / 0.01). Update the comment to the correct exponent/scaling (the asserted value 29 looks consistent with2^(3/2)/0.1).
unitary = builder.run(hamiltonian, time=1.0)
container = unitary.get_container()
# one_norm = 2, N = ceil(2^1/2 / 0.01) = 29
assert container.step_reps == 29
python/src/qdk_chemistry/algorithms/time_evolution/builder/trotter_error.py:87
trotter_steps_naivenow supportsorder=2(seeorder not in {1,2}and the generalized power-law), but the docstring above still documents only the first-ordert^2/εscaling. Please update the docstring’s math to cover both orders (or explicitly state it’s the order=1 formula).
if order not in {1, 2}:
raise NotImplementedError(
f"Trotter step estimation for order {order} is not yet implemented. "
"Only orders 1 or 2 are currently supported."
)
real_terms = hamiltonian.get_real_coefficients(tolerance=weight_threshold)
one_norm = sum(abs(coeff) for _, coeff in real_terms)
return max(1, math.ceil((one_norm ** (1 + 1 / order) * time ** (1 + 1 / order)) / (target_accuracy ** (1 / order))))
python/src/qdk_chemistry/algorithms/time_evolution/builder/trotter_error.py:110
- The docstring currently mixes a ceil-expression for
N_1with a raw error-term expression forN_2, and the nested-commutator expression has inconsistent indices/brackets. Since this function returns anN(step count), the math section should present the solvedNformula for order=2 (including the expected1/N^2scaling for second-order) to avoid misleading readers.
.. math::
N_1 = \left\lceil \frac{t^2}{2\epsilon}
\sum_{j<k} \lVert [\alpha_j P_j,\, \alpha_k P_k] \rVert
\right\rceil
N_2 = \frac{t^3}{3!} \sum_{j < k < l}
\lVert \lvert [\alpha_j P_l,\, [\alpha_j P_j,\, \alpha_k P_k] \rVert ] \rVert
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
python/src/qdk_chemistry/algorithms/time_evolution/builder/trotter_error.py
Outdated
Show resolved
Hide resolved
python/src/qdk_chemistry/algorithms/time_evolution/builder/trotter_error.py
Outdated
Show resolved
Hide resolved
wavefunction91
requested changes
Mar 4, 2026
python/src/qdk_chemistry/algorithms/time_evolution/builder/trotter.py
Outdated
Show resolved
Hide resolved
wavefunction91
changed the title
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
Comments suppressed due to low confidence (2)
python/src/qdk_chemistry/algorithms/time_evolution/builder/trotter.py:152
- The method now handles both order=1 and order=2, but the docstring still describes only first-order Trotter and gives the first-order product formula. Please update this docstring to describe that the implementation supports orders 1 and 2 (and document the Strang/symmetric construction for order=2), or split into separate methods with accurate docstrings.
def _trotter(self, qubit_hamiltonian: QubitHamiltonian, time: float) -> TimeEvolutionUnitary:
r"""Construct the time evolution unitary using first-order Trotter decomposition.
The First Order Trotter method approximates the time evolution operator :math:`e^{-iHt}`
by decomposing the Hamiltonian H into a sum of terms and using the product formula:
:math:`e^{-iHt} \approx \left[\prod_i e^{-iH_i t/n}\right]^n`, where n is the number of divisions.
python/src/qdk_chemistry/algorithms/time_evolution/builder/trotter_error.py:1
- The newly added
N_2math block appears malformed (mismatched\\lVert,\\lvert, brackets) and the indices/term structure look inconsistent. Since this docstring is user-facing, please rewrite the second-order bound section with a correct and readable formula, and ideally align it with howcommutator_bound_second_order()is defined (including thet^3/(12 N^2)scaling used in the implementation).
r"""Trotter error bound estimation for accuracy-aware parameterization.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
python/src/qdk_chemistry/algorithms/time_evolution/builder/trotter.py
Outdated
Show resolved
Hide resolved
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
python/src/qdk_chemistry/algorithms/time_evolution/builder/trotter.py:153
- The
_trottermethod docstring still describes only the first-order product formula, but the method now also implements second-order decomposition depending on theordersetting (via_decompose_trotter_step). Please update the docstring to reflect that this method supports orders 1 and 2 (and describe the second-order/Strang form) so callers reading docs aren't misled.
def _trotter(self, qubit_hamiltonian: QubitHamiltonian, time: float) -> TimeEvolutionUnitary:
r"""Construct the time evolution unitary using first-order Trotter decomposition.
The First Order Trotter method approximates the time evolution operator :math:`e^{-iHt}`
by decomposing the Hamiltonian H into a sum of terms and using the product formula:
:math:`e^{-iHt} \approx \left[\prod_i e^{-iH_i t/n}\right]^n`, where n is the number of divisions.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
python/src/qdk_chemistry/algorithms/time_evolution/builder/trotter_error.py
Outdated
Show resolved
Hide resolved
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (1)
python/src/qdk_chemistry/algorithms/time_evolution/builder/trotter.py:152
_trotteris now used for both order=1 and order=2 (see_run_impl), but its docstring still describes only first-order Trotter and the first-order product formula. Update the docstring (and any referenced formula text) to reflect that this method constructs the configured order (1 or 2), or split into separate order-specific helpers with matching docstrings.
r"""Construct the time evolution unitary using first-order Trotter decomposition.
The First Order Trotter method approximates the time evolution operator :math:`e^{-iHt}`
by decomposing the Hamiltonian H into a sum of terms and using the product formula:
:math:`e^{-iHt} \approx \left[\prod_i e^{-iH_i t/n}\right]^n`, where n is the number of divisions.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
python/src/qdk_chemistry/algorithms/time_evolution/builder/trotter_error.py
Outdated
Show resolved
Hide resolved
…er' into feature/agamshayit/2ndordertrotter
wavefunction91
requested changes
Mar 13, 2026
python/src/qdk_chemistry/algorithms/time_evolution/builder/trotter.py
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
Comments suppressed due to low confidence (1)
python/src/qdk_chemistry/algorithms/time_evolution/builder/trotter.py:150
Trotternow acceptsorder1 or 2, but_trotter’s docstring still describes first-order decomposition and theS_1formula. Update the docstring (and any referenced formula) to describe order-dependent behavior or rename the method/commentary accordingly so users aren’t misled.
def _trotter(self, qubit_hamiltonian: QubitHamiltonian, time: float) -> TimeEvolutionUnitary:
r"""Construct the time evolution unitary using first-order Trotter decomposition.
The First Order Trotter method approximates the time evolution operator :math:`e^{-iHt}`
by decomposing the Hamiltonian H into a sum of terms and using the product formula:
:math:`e^{-iHt} \approx \left[\prod_i e^{-iH_i t/n}\right]^n`, where n is the number of divisions.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
python/src/qdk_chemistry/algorithms/time_evolution/builder/trotter_error.py
Show resolved
Hide resolved
python/src/qdk_chemistry/algorithms/time_evolution/builder/trotter_error.py
Show resolved
Hide resolved
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (1)
python/src/qdk_chemistry/algorithms/time_evolution/builder/trotter.py:149
_trotteris now used for both order 1 and order 2 (via_decompose_trotter_stepreading settings), but its docstring still says “first-order Trotter decomposition” and shows only the first-order product formula. Update this docstring to reflect order-dependent behavior (first-order vs second-order/Strang splitting).
def _trotter(self, qubit_hamiltonian: QubitHamiltonian, time: float) -> TimeEvolutionUnitary:
r"""Construct the time evolution unitary using first-order Trotter decomposition.
The First Order Trotter method approximates the time evolution operator :math:`e^{-iHt}`
by decomposing the Hamiltonian H into a sum of terms and using the product formula:
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
python/src/qdk_chemistry/algorithms/time_evolution/builder/trotter_error.py
Show resolved
Hide resolved
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.
Added support for second-order Trotter time evolution. Implemented the Naive and commutator-based error bounds for the second-order Trotter formula and added some tests