Introduce PauliLindbladMap #14383
Introduce PauliLindbladMap #14383
Conversation
Pull Request Test Coverage Report for Build 15448742213Warning: 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 |
|
One or more of the following people are relevant to this code:
|
This is a roll-up of Dan's work of building the `PauliLindbladMap` on top of [1]. [1]: 09e0de0: Add QubitSparsePauli and QubitSparsePauliList classes to quantum_info Co-authored-by: Samuele Ferracin <sam.ferracin@ibm.com>
63477fd to
a420620
Compare
|
I rebased and rolled up this PR over the just-merged #14283, just to help speed-up the review process from my side - Dan/Sam, if you need to pull locally, your branches will be out of sync, so you'll need to make sure your branches are correctly at the previous tip commit 63477fd (so we don't drop any changes you might have locally), then do If you're worried, you can branch off locally first |
There was a problem hiding this comment.
Thanks for this Dan - I've left some comments on this here. I can't comment too much on the exact maths and whatnot of the object without spending a fair amount more time thinking about it, so I'm largely trusting that the three (four?) of you have got it right for what you wanted.
My biggest concerns here are that the way the Rust/Python split works with with compound object of PauliLindbladMap and the inner QubitSparsePauliList aren't right - there have been problems introduced as this moved beyond the structures that SparseObservable had laid the groundwork for. I've left the comments here for first discussion, and I'll think about more asynchronously about some of the points I raised here.
| #[pyclass(name = "QubitSparsePauli", frozen, module = "qiskit.quantum_info")] | ||
| #[derive(Clone, Debug)] | ||
| pub struct PyQubitSparsePauli { | ||
| inner: QubitSparsePauli, | ||
| pub inner: QubitSparsePauli, | ||
| } |
There was a problem hiding this comment.
This is unsafe from a memory-safety perspective: you have unsafe blocks in PyQubitSparsePauli that rely on the inner object being immutable (or more specifically, its internal allocations not moving) within the lifetime of the outer object.
Specifically: the methods that return Numpy arrays that are backed by the Rust-space allocations can become memory-unsafe with this, if a user of PyQubitSparsePauli were to use the pub visibility on inner to move it and drop it while a Numpy array was still live. The Python-space lifetimes only protect the existence of the outer PyQubitSparsePauli, and not its inner object.
What do you need the pub for here? We can likely find ways that give you the access you need that remain safe.
As a follow-up: I realise that this is way too easy to get wrong now, and you have to have huge amounts of knowledge in mind about the entirety of the SparseObservable data structures to keep this safe. I'll follow up with a PR to SparseObservable and to all these new structures that modifies how the Numpy array-access sharing is done to make this much harder to get wrong, at the cost of a small runtime penalty that I expect will be close-enough to unmeasurable anyway.
There was a problem hiding this comment.
In the follow up PR I had instances of wanting to access the rust-space object QubitSparsePauli from within methods of PyPauliLindbladMap.
| pub fn raw_parts_from_sparse_list( | ||
| iter: Vec<(String, Vec<u32>)>, | ||
| num_qubits: u32, | ||
| ) -> Result<RawParts, LabelError> { |
There was a problem hiding this comment.
It's fine to split it out like this. That said, if you prefer, you can provide a safe and no-cost way of achieving this from QubitSparsePauli itself, like so:
impl QubitSparsePauliList {
pub fn take(self) -> (Vec<Pauli>, Vec<u32>, Vec<usize>) {
(self.paulis, self.indices, self.boundaries)
}
}then you can have leave QubitSparsePauliList::from_raw_parts as is, and have the new callers of raw_parts_from_sparse_list be like
fn whatever(num_qubits: u32) -> Result<T, LabelError> {
let (paulis, indices, boundaries) = QubitSparsePauliList::from_raw_parts(...)?.take();
}Having take take self by ownership means that you can absolve yourself of coherence issues in the API after that - it's a way of providing safe API access to completely destructure the struct into its constituent parts without having to give pub access to fields that can't be pub while the struct is still implying the semantics.
There was a problem hiding this comment.
The main reason I split this method out is that the same code is needed in PauliLindbladMap.
| /// A Pauli Lindblad map that stores its data in a qubit-sparse format. Note that gamma, | ||
| /// probabilities, and non_negative_rates is |
There was a problem hiding this comment.
Looks like something's got truncated here.
There was a problem hiding this comment.
Thanks - just completed the sentence.
| /// Create a new Pauli Lindblad map from the raw components that make it up. | ||
| pub fn new_from_raw_parts( | ||
| num_qubits: u32, | ||
| rates: Vec<f64>, | ||
| paulis: Vec<Pauli>, | ||
| indices: Vec<u32>, | ||
| boundaries: Vec<usize>, | ||
| ) -> Result<Self, CoherenceError> { | ||
| if rates.len() + 1 != boundaries.len() { | ||
| return Err(CoherenceError::MismatchedTermCount { | ||
| rates: rates.len(), | ||
| qspl: boundaries.len() - 1, | ||
| }); | ||
| } | ||
| let qubit_sparse_pauli_list: QubitSparsePauliList = | ||
| QubitSparsePauliList::new(num_qubits, paulis, indices, boundaries)?; | ||
| Self::new(rates, qubit_sparse_pauli_list) | ||
| } |
There was a problem hiding this comment.
The idea of from_raw_parts in SparseObservable is to be as close to a zero-cost constructor as possible.
This method is identical both in function and performance to PauliLindbladMap::new(rates, QubitSparsePauliList::new(num_qubits, paulis, indices, boundaries)?), and it's (imo) more idiomatic rust not to hide that fact, but just have the caller spell it themselves, which it's not much more typing already.
There was a problem hiding this comment.
True - dropping this method and updating the one place it is used to use your suggested line.
| /// Create a new [PauliLindbladMap] without checking data coherence. | ||
| /// | ||
| /// # Safety | ||
| /// | ||
| /// It is up to the caller to ensure that the data-coherence requirements, as enumerated in the | ||
| /// struct-level documentation, have been upheld. | ||
| #[inline(always)] | ||
| pub unsafe fn new_unchecked( | ||
| rates: Vec<f64>, | ||
| qubit_sparse_pauli_list: QubitSparsePauliList, | ||
| ) -> Self { | ||
| let (gamma, probabilities, non_negative_rates) = derived_values_from_rates(&rates); | ||
| Self { | ||
| rates, | ||
| qubit_sparse_pauli_list, | ||
| gamma, | ||
| probabilities, | ||
| non_negative_rates, | ||
| } | ||
| } |
There was a problem hiding this comment.
Typically the purpose of unsafe constructors is to allow zero-computation methods of construction to callers who don't have privileged access to the internals of the struct. This method still does some fairly heavy calculations (including some memory allocations), so unless it's super necessary, it's possibly not worth the risk of adding an unsafe method.
I don't think you use this anywhere, so it might be cleanest just to remove it.
There was a problem hiding this comment.
This does get used in the followup PR, though could easily be replaced. It's true that all it does is not check if rates.len() == qubit_sparse_pauli_list.len().
There was a problem hiding this comment.
Okay, deleted this.
| /// Return the pauli labels of the term as string. | ||
| /// | ||
| /// The pauli labels will match the order of :attr:`.GeneratorTerm.indices`, such that the | ||
| /// i-th character in the string is applied to the qubit index at ``term.indices[i]``. | ||
| /// | ||
| /// Returns: | ||
| /// The non-identity bit terms as concatenated string. | ||
| fn pauli_labels<'py>(&self, py: Python<'py>) -> Bound<'py, PyString> { | ||
| let string: String = self | ||
| .inner | ||
| .paulis() | ||
| .iter() | ||
| .map(|bit| bit.py_label()) | ||
| .collect(); | ||
| PyString::new(py, string.as_str()) | ||
| } |
There was a problem hiding this comment.
Similar question about these guys as with QubitSparsePauli (where you deleted it). I imagine you've got more desire for it here? If so: it'd be nice to include an example in the documentation illustrating what you mean on lines 506-507.
There was a problem hiding this comment.
Actually I also think this is unnecessary. Deleted :P .
Edit: I had originally deleted this but decided to put it back. I've added an example.
| .inner | ||
| .read() | ||
| .map_err(|_| InnerReadError)?; | ||
| let inner = PauliLindbladMap::new(rates.clone(), qubit_sparse_pauli_list.clone())?; |
There was a problem hiding this comment.
You probably don't need to clone the rates here. It's not super ideal that we have to clone the QubitSparsePauliList on input, but not the end of the world right now - can always be addressed later.
There was a problem hiding this comment.
Okay dropping the cloning of the rates. Yeah I don't like cloning the operator list either but I wasn't sure how to get around that. Is it correct that if we demand it is passed as a mutable reference we could guarantee no one else has access to change it anymore? (What would be idiomatic here?)
| /// The map's qubit sparse pauli list. | ||
| #[getter] | ||
| fn get_qubit_sparse_pauli_list(&self) -> PyQubitSparsePauliList { | ||
| let inner = self.inner.read().unwrap(); | ||
| inner.qubit_sparse_pauli_list.clone().into() | ||
| } |
There was a problem hiding this comment.
This is almost certainly not what you're after for performance: the qubit_sparse_pauli_list itself is cloned on each attribute access; it's like you're doing a Python-space deepcopy on the underlying list each time you access the attribute. This is going to make plm.qubit_sparse_pauli_list attribute accesses really expensive in Python space.
It's not immediately clear to me what your best bet is going to be here - you can't just expose internals of Rust objects to Python space safely without having designed in the capability from a fairly early point.
If you can get away with it for the first release, you're probably best making this a regular method (so not a getter), giving it a slightly different name to make it clear it makes a copy, and relying on callers to eat the cost of the copy if they need the internal access. We can potentially work out some new method of structuring the data types later to make it easier to make a raw getter in the future.
In particular: it would be easier to imagine such a situation if the derived quantities from rates aren't stored, but are calculated only at the point of use. That's not necessary, but my first thoughts for improving the attribute access here want it. We can find a way around that if we need to.
There was a problem hiding this comment.
Okay sounds good. I've dropped the getter decorator and renamed the method to:
get_qubit_sparse_pauli_list_copy
| /// .. math:: | ||
| /// | ||
| /// \Lambda = \prod_{P \in K}\exp\left(\lambda_P(P \cdot P - \cdot)\right). |
There was a problem hiding this comment.
Minor style: here and everywhere in the docs, I find the \cdot very hard to read; to me, it's a multiplication marker. Perhaps \circ is a better choice? Compare this one using \cdot:
to this one using \circ:
There was a problem hiding this comment.
I don't feel strongly about this personally. I naturally prefer \cdot as I've always used \circ to refer to composition, but I also agree that in this case the \cdot is weird looking appearing by itself after a minus sign. I'll change it to \circ. I've also changed the
| /// The probabilities for the map. | ||
| #[getter] | ||
| fn get_probabilities(slf_: Bound<Self>) -> Bound<PyArray1<f64>> { | ||
| let borrowed = &slf_.borrow(); | ||
| let inner = borrowed.inner.read().unwrap(); | ||
| let probabilities = inner.probabilities(); | ||
| let arr = ::ndarray::aview1(probabilities); | ||
| // SAFETY: in order to call this function, the lifetime of `self` must be managed by Python. | ||
| // We tie the lifetime of the array to `slf_`, and there are no public ways to modify the | ||
| // `Box<[u32]>` allocation (including dropping or reallocating it) other than the entire | ||
| // object getting dropped, which Python will keep safe. | ||
| let out = unsafe { PyArray1::borrow_from_array(&arr, slf_.into_any()) }; | ||
| out.readwrite().make_nonwriteable(); | ||
| out | ||
| } |
There was a problem hiding this comment.
The safety guarantees on these really don't hold in the same way that they did in the smaller scale objects. I'm a bit worried about this.
There was a problem hiding this comment.
Thoughts on how to resolve this?
There was a problem hiding this comment.
I ended up dropping the #[getter] decorator so that this no longer looks like an attribute. I've done the same with get_gamma.
|
@jakelishman I believe I've addressed all of the comments except for the final one about concern with the I've also added the method |
This defines the interface of these functions to be regulard methods that compute quantities. This includes having `probabilities` return a copy of a Numpy array, which is what will be needed in follow-ups anyway; we don't want the Python interface to leak that certain quantities are actually currently cached (since that will change in the future). A couple of Rust-space methods are made more "Rust-y" as a side effect.
There was a problem hiding this comment.
Ok, I think this is all safe for the interface, in preparation for more completely moving to #14528 in Qiskit 2.2.
| /// The rates of each abstract term in the generator sum. This has as many elements as | ||
| /// terms in the sum. | ||
| rates: Vec<f64>, | ||
| /// A list of qubit sparse Paulis corresponding to the rates | ||
| qubit_sparse_pauli_list: QubitSparsePauliList, | ||
| /// The gamma parameter. | ||
| gamma: f64, | ||
| /// Probability of application of a given Pauli operator in product form. Note that if the | ||
| /// corresponding rate is less than 0, this is a quasi-probability. | ||
| probabilities: Vec<f64>, | ||
| /// List of boolean values for the statement rate >= 0 for each rate in rates. | ||
| non_negative_rates: Vec<bool>, |
There was a problem hiding this comment.
For the future: we intend to split these out in a future version of PauliLindbladMap, to make the objects lighter to manipulate, especially want sampling under different rates/masks. See #14528.
They're staying in the object for this PR, because it still works, and we're just not exposing them publicly to Python space in the interface.
* Introduce PauliLindbladMap This is a roll-up of Dan's work of building the `PauliLindbladMap` on top of [1]. [1]: 09e0de0: Add QubitSparsePauli and QubitSparsePauliList classes to quantum_info Co-authored-by: Samuele Ferracin <sam.ferracin@ibm.com> * making suggested changes * formatting and re-adding pauli_labels method * formatting * fixing forgotten inner issue * Fix minor documentation copy/paste * Make `gamma()` and `probabilities()` regular computing methods This defines the interface of these functions to be regulard methods that compute quantities. This includes having `probabilities` return a copy of a Numpy array, which is what will be needed in follow-ups anyway; we don't want the Python interface to leak that certain quantities are actually currently cached (since that will change in the future). A couple of Rust-space methods are made more "Rust-y" as a side effect. * Fix clippy --------- Co-authored-by: Samuele Ferracin <sam.ferracin@ibm.com> Co-authored-by: Jake Lishman <jake.lishman@ibm.com>
Summary
This is the second PR in a series of 3 introducing
QubitSparsePauli,QubitSparsePauliList, andPauliLindbladMap. The first PR, which this PR depends on, is #14283 .This introduces the data structure of the class
PauliLindladMap, building onQubitSparsePauliList.Details and comments
This depends on #14283