Add accuracy-aware Trotter parameterization with unified commutator infrastructure#366
Open
Add accuracy-aware Trotter parameterization with unified commutator infrastructure#366
Conversation
Co-authored-by: wavefunction91 <6208701+wavefunction91@users.noreply.github.com>
- Add pauli_commutation module with Pauli string commutation checks - Add trotter_error module with naive and commutator-based error bounds - Update Trotter class with target_accuracy and error_bound parameters - Add comprehensive tests for all new functionality Co-authored-by: wavefunction91 <6208701+wavefunction91@users.noreply.github.com>
…irectory Co-authored-by: wavefunction91 <6208701+wavefunction91@users.noreply.github.com>
…sing Co-authored-by: wavefunction91 <6208701+wavefunction91@users.noreply.github.com>
Co-authored-by: wavefunction91 <6208701+wavefunction91@users.noreply.github.com>
Copilot
AI
changed the title
[WIP] Add accuracy-aware Trotter parameterization for Lie-Trotter decomposition
Add accuracy-aware Trotter parameterization with commutator bounds
Feb 16, 2026
Collaborator
|
@copilot merge with main and resolve conflicts, noting that commutator infrastructure was also added in another PR. Make this infrastructure unified and formal / visible and integrate into the new Trotter implementation. |
…n module Resolve merge conflicts with main (PR #354 - qDRIFT/PartiallyRandomized). Lift commutation functions from QDrift into shared pauli_commutation module: - do_pauli_terms_commute / do_pauli_terms_qw_commute (map-based) - do_pauli_strings_qw_commute (label-based, new) - get_commutation_checker factory QDrift delegates to shared module. Trotter uses main's refactored _decompose_trotter_step with is_hermitian/get_real_coefficients helpers. Co-authored-by: wavefunction91 <6208701+wavefunction91@users.noreply.github.com>
Co-authored-by: wavefunction91 <6208701+wavefunction91@users.noreply.github.com>
… directly Replace QDrift._pauli_terms_commute/_pauli_terms_qw_commute/_get_commutation_checker wrappers with direct calls to shared pauli_commutation module functions. Update PartiallyRandomized to import get_commutation_checker directly. Co-authored-by: wavefunction91 <6208701+wavefunction91@users.noreply.github.com>
Co-authored-by: wavefunction91 <6208701+wavefunction91@users.noreply.github.com>
Contributor
Author
Merged with main and unified the commutation infrastructure in commit Changes:
Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
from qdk_chemistry.algorithms.time_evolution.builder.qdrift import QDrift Test that QDrift uses the shared commutation functionsfrom qdk_chemistry.algorithms.time_evolu` (dns block)
|
…nfrastructure Co-authored-by: wavefunction91 <6208701+wavefunction91@users.noreply.github.com>
Copilot
AI
changed the title
Add accuracy-aware Trotter parameterization with commutator bounds
Add accuracy-aware Trotter parameterization with unified commutator infrastructure
Feb 18, 2026
Collaborator
|
@copilot Fix the failing test |
…uli_commutation module The test referenced QDrift._pauli_terms_qw_commute which was removed when we unified the commutation infrastructure. Updated to use the public do_pauli_terms_qw_commute from the pauli_commutation module. Co-authored-by: wavefunction91 <6208701+wavefunction91@users.noreply.github.com>
Contributor
Author
Fixed in |
wavefunction91
requested changes
Feb 18, 2026
python/src/qdk_chemistry/algorithms/time_evolution/builder/__init__.py
Outdated
Show resolved
Hide resolved
python/src/qdk_chemistry/algorithms/time_evolution/builder/pauli_commutation.py
Outdated
Show resolved
Hide resolved
python/src/qdk_chemistry/algorithms/time_evolution/builder/pauli_commutation.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.py
Outdated
Show resolved
Hide resolved
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
… and error bounds - Move pauli_commutation.py to utils/ (top-level utility module) - Remove re-exports from builder/__init__.py - commutator_bound_first_order now accepts QubitHamiltonian - Rename tolerance -> weight_threshold in Trotter - Replace num_trotter_steps with unified num_steps override - Add trotter_steps() order-aware dispatcher with stubs for higher order - trotter_steps_naive/commutator now accept QubitHamiltonian - Move qw_commute tests from qdrift test to pauli_commutation test - Keep backward-compatible re-export shim in builder/pauli_commutation.py Co-authored-by: wavefunction91 <6208701+wavefunction91@users.noreply.github.com>
Contributor
Author
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
class MockModule(types.ModuleType): core =` (dns block)
|
📊 Coverage Summary
Detailed Coverage ReportsC++ Coverage DetailsPython Coverage DetailsPybind11 Coverage Details |
wavefunction91
requested changes
Feb 18, 2026
python/src/qdk_chemistry/algorithms/time_evolution/builder/pauli_commutation.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.py
Outdated
Show resolved
Hide resolved
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
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
Show resolved
Hide resolved
python/src/qdk_chemistry/algorithms/time_evolution/builder/trotter_error.py
Show resolved
Hide resolved
…shim - Delete backward-compat shim builder/pauli_commutation.py - Rename do_pauli_strings_* → do_pauli_labels_* (label-based checks) - Rename do_pauli_terms_* → do_pauli_maps_* (map-based checks) - Remove trotter_steps() dispatcher; add order param to trotter_steps_naive/trotter_steps_commutator directly - Remove section comment block separators - Clean up module docstrings (remove unnecessary remarks) - Rename num_steps → num_divisions (divisions within a step) - Remove backward-compatibility framing from Trotter docs Co-authored-by: wavefunction91 <6208701+wavefunction91@users.noreply.github.com>
Contributor
Author
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
wavefunction91
requested review from
ConradJohnston and
nabbelbabbel
as code owners
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds accuracy-aware Trotter parameterization with automatic step-count determination and unifies Pauli commutation checking across time-evolution builders. The changes address the issue where Lie-Trotter decomposition used a fixed number of steps regardless of evolution time, which could produce inaccurate results for large t.
Changes:
- Introduces shared
qdk_chemistry.utils.pauli_commutationmodule consolidating all Pauli commutation checks from Trotter, QDrift, and PartiallyRandomized builders - Adds
trotter_error.pywith naive and commutator-based error bound estimation functions for computing required Trotter step counts - Extends Trotter builder with
target_accuracyparameter for automatic step-count determination using either commutator (tighter, default) or naive bounds, while maintaining backward compatibility
Reviewed changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
python/src/qdk_chemistry/utils/pauli_commutation.py |
New unified commutation module with label-based and map-based checkers, plus commutator bound computation |
python/src/qdk_chemistry/algorithms/time_evolution/builder/trotter_error.py |
New error bound estimation functions for naive and commutator-based step count calculations |
python/src/qdk_chemistry/algorithms/time_evolution/builder/trotter.py |
Extended API with target_accuracy, num_divisions, error_bound, weight_threshold parameters (renames num_trotter_steps→num_divisions, tolerance→weight_threshold) |
python/src/qdk_chemistry/algorithms/time_evolution/builder/qdrift.py |
Refactored to use shared commutation utilities, removed duplicate commutation methods |
python/src/qdk_chemistry/algorithms/time_evolution/builder/partially_randomized.py |
Updated to import get_commutation_checker from shared utilities |
python/tests/test_pauli_commutation.py |
Comprehensive tests for all commutation utilities and commutator bound computation |
python/tests/test_trotter_error.py |
Tests for naive and commutator error bound estimation functions |
python/tests/test_time_evolution_trotter.py |
Extended tests for accuracy-aware Trotter parameterization and backward compatibility |
python/tests/test_time_evolution_qdrift.py |
Removed tests for now-shared commutation methods |
.gitignore |
Removed incorrect python/VERSION entry (should be tracked, not ignored) |
.github/scripts/check_license_headers.py |
Updated regex to support raw docstrings (r-prefix) needed by trotter_error.py |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+43
to
+51
| def __init__( | ||
| self, | ||
| order: int = 1, | ||
| *, | ||
| target_accuracy: float | None = None, | ||
| num_divisions: int | None = None, | ||
| error_bound: str = "commutator", | ||
| weight_threshold: float = 1e-12, | ||
| ): |
There was a problem hiding this comment.
The factory_list.ipynb example notebook documents the old parameter names num_trotter_steps and tolerance for the Trotter builder. These have been renamed to num_divisions and weight_threshold respectively. The notebook should be updated to reflect the current API.
Comment on lines
+43
to
+51
| def __init__( | ||
| self, | ||
| order: int = 1, | ||
| *, | ||
| target_accuracy: float | None = None, | ||
| num_divisions: int | None = None, | ||
| error_bound: str = "commutator", | ||
| weight_threshold: float = 1e-12, | ||
| ): |
There was a problem hiding this comment.
This PR introduces breaking changes to the Trotter builder API. The CHANGELOG should be updated to document:
- Renamed parameters:
num_trotter_steps→num_divisions,tolerance→weight_threshold - New keyword-only parameters:
target_accuracy,error_bound - Migration guide for users updating from previous versions
This documentation is important for users to understand the changes and update their code accordingly.
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.
The Lie-Trotter decomposition currently uses a fixed number of steps regardless of evolution time, producing silently inaccurate unitaries for large
t. This adds automatic step-count determination from a user-specified error tolerance, using both a naive bound and the tighter commutator-based bound from Childs et al. (2021).Additionally, this PR unifies all Pauli commutation checking into a single shared utility module (
qdk_chemistry.utils.pauli_commutation), replacing duplicated implementations that were spread across individual builder classes.Unified commutation module —
utils/pauli_commutation.pyServes as the single source of truth for all Pauli commutation checks across time-evolution builders (Trotter, QDrift, PartiallyRandomized). Users import directly from
qdk_chemistry.utils.pauli_commutation."XYZII"):do_pauli_labels_commute(a, b)— standard commutation check (even number of anticommuting positions)do_pauli_labels_qw_commute(a, b)— stricter qubit-wise commutation (no anticommuting positions)dict[int, str]qubit→Pauli mappings):do_pauli_maps_commute(a, b)— general commutationdo_pauli_maps_qw_commute(a, b)— qubit-wise commutationget_commutation_checker("general" | "qubit_wise")— returns the appropriate map-based checkercommutator_bound_first_order(hamiltonian)— accepts aQubitHamiltonianand computesΣ_{j<k} ‖[αⱼPⱼ, αₖPₖ]‖over all anticommuting pairsTrotter error estimation —
trotter_error.pyFunctions for computing required Trotter step counts, with an
orderparameter for future extension to arbitrary product-formula order (currently raisesNotImplementedErrorfor order > 1):trotter_steps_naive(hamiltonian, time, target_accuracy, *, order)— triangle-inequality boundtrotter_steps_commutator(hamiltonian, time, target_accuracy, *, order)— commutator-based bound (tighter)All functions accept
QubitHamiltoniandirectly.Updated
Trotterbuildertarget_accuracy(keyword-only): When set, automatically computes the minimum number of Trotter divisionsNto achieve the requested accuracy.num_divisions(keyword-only): Explicit override for the number of Trotter divisions within a step. When bothnum_divisionsandtarget_accuracyare given, the larger value is used.error_bound:"commutator"(default, tighter) or"naive".weight_threshold: Absolute threshold for filtering small Hamiltonian coefficients.Trotter()with no arguments defaults to 1 division, matching previous behavior.Other changes
QDrift; it now imports directly from the shared utility module.PartiallyRandomizedto importget_commutation_checkerfrom the shared module.Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.