Refactor/abstract qubit sparse pauli-type interfaces#14901
Open
DanPuzzuoli wants to merge 33 commits into
Open
Refactor/abstract qubit sparse pauli-type interfaces#14901DanPuzzuoli wants to merge 33 commits into
DanPuzzuoli wants to merge 33 commits into
Conversation
…ase as group phase
Collaborator
|
One or more of the following people are relevant to this code:
|
Pull Request Test Coverage Report for Build 16950745364Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
aaryav-3
reviewed
Nov 7, 2025
| order.sort_unstable_by_key(|a| indices[*a]); | ||
| let paulis = order.iter().map(|i| paulis[*i]).collect(); | ||
| let mut sorted_indices = Vec::<u32>::with_capacity(order.len()); | ||
| for i in order { |
There was a problem hiding this comment.
Thank you for your contribution! This PR is a great refactoring for pauli-related structures, bringing them under one umbrella macro for the python interface. Your code LGTM. But I have one minor refactoring suggestion for enhanced code clarity and minimization.
This sorting logic here may be written as:
let mut order: Vec<_> = indices.iter().copied().enumerate().collect();
order.sort_unstable_by_key(|(_, index)| *index);
let (sorted_indices, sorted_paulis): (Vec<u32>, Vec<Pauli>) = order
.into_iter()
.map(|(i, index)| (index, paulis[i]))
.unzip();and perhaps after the index sorting a .window(2) based check can be done to validate an inconsistency in the ascending order, as:
if order.windows(2).any(|i| i[0].1 >= i[1].1) {
return Err(CoherenceError::UnsortedIndices.into());
}
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.
Summary
This PR follows up #14759 (which adds phased versions of
QubitSparsePauliandQubitSparsePauliList), and anticipates adding even more similar classes (QubitSparsePauliSetandPhasedQubitSparsePauliSet). These classes all share overlapping behaviour, and also have high maintenance cost due to needing to define both rust and python interfaces. As such it is worth "getting it right" by taking some time to define their interfaces more carefully and to refactor their shared behaviour into a single location/implementation where possible.The PR currently contains an initial pass for the "collection-type" objects: currently the list classes
QubitSparsePauliListandPhasedQubitSparsePauliList, the unimplemented "set" objects, and alsoPauliLindbladMap. I view these as classes that store a collection of qubit-sparse Paulis along with contextual data (e.g.phase, orrates,probabilities, etc...). I think one of the things that's unclear to me is how much the "contextual data" will impact the ability to centralize functionality (i.e. cases of very similar behaviour but where it doesn't make sense to use the same signature), but in any case I think it's clearly useful to do this to the degree that it is possible.To do this we need to define both rust and python interfaces. As suggested by @jakelishman , it seems the technical path forward is:
traits to define rust interfaces and default implementations.traits unfortunately don't interact well with pyo3.Details and comments
Rust interface
For the rust interface, I've currently defined a
QubitSparsePauliListLiketrait in `qubit_sparse_pauli.rs", which starts with the interface definitions (code from @jakelishman ):For
QubitSparsePauliListthese returnself, but other classes will need to return the internally storedQubitSparsePauliList(or simply generate it if it is no longer part of the interal data structure).From there, "property" methods are defined in terms of these:
Lastly, some methods that are primarily based around modifying the qubit-sparse pauli structure:
Both
QubitSparsePauliListandPhasedQubitSparsePaulilist have implementations of this trait. Note that the implementations of the last three methods would be extremely similar for all of the considered classes: calling the same method on the underlyingQubitSparsePauliListand putting the result, along with copies of the context-dependant data in each class implementing the trait, into a new instance of said class. If there is a way to write a default implementation of such functions that could copy over all other data, irrespective of what it is, that'd be very nice.Python interface
This is quite minimal but I've currently defined:
in the
src/pauli_lindblad_map/mod.rsfile. I've defined there because the rules for where macros live are more constrained; this was the first thing I tried that compiled but presumably we'll want to find a better place for it. I haven't included it yet but I'll addapply_layout,drop_paulis/keep_paulis, anddrop_qubits/keep_qubits(I haven't looked super closely at what else).The python version of the classes again have a lot of similarly defined methods, but with slightly different signatures. E.g. many of the constructors are similar in spirit, and iteration/indexing over the constituent units is similar. I figure these things may be handle-able somehow with macros but it'll require a better understanding of rust typing than I have.
Other classes
The other classes to consider are
QubitSparsePauli,PhasedQubitSparsePauli, andGeneratorTerm(the "single-element" version of aPauliLindbladMap). These classes are much simpler so I think the maintenance cost is less of a concern, but it'd be nice to do a similartrait/macro setup for them.