Split `PySparseObservable` off `SparseObservable` #13595

Cryoris · 2024-12-23T10:50:33Z

Summary

Closes #13594 to prepare for SparseObservable's C API. This change has been tested with our basic C API for SparseObservable, which will come in a separate PR to keep the review load in balance 🙂

Details and comments

This PR splits the sparse observable class into a Rust-only SparseObservable struct and a PySparseObservable, which serves as Python interface. As suggested in #13391, the Python interface keeps an Arc to a read-write-locked SparseObservable. The API Change label is only due to some minuscule change in an error message, the Python interface remains unchanged.

The implementation is based on

#[pyclass(name = "SparseObservable", ...)]  // exposed as qiskit.quantum_info.SparseObservable, as before
struct PySparseObservable {
    // This class keeps a pointer to a pure Rust-SparseTerm and serves as interface from Python.
    inner: Arc<RwLock<SparseObservable>>,
}

and methods on PySparseObservable first acquire the read- or write-lock to perform actions on the inner data. For example, implementing transpose becomes

    fn transpose(&self) -> PyResult<Self> {
        // acquire the read lock, mapping the PoisonError into our own error that can be cast to a PyErr
        let inner = self.inner.read().map_err(|_| InnerReadError)?;

        // perform the action
        let result = inner.transpose();  
        
        // return a new Arc<RwLock> (if we did an inplace operation, we would just return nothing)
        Ok(Self { inner: Arc::new(RwLock::new(result)) })
    }

Some notes/questions:

For SparseTerm we analogously split off PySparseTerm, since it can be returned to Python. The view/mutable view versions are not returned to Python and don't need a specific interface.
We couldn't implement IntoPy to PoisonError (coming from RwLock::read/write), so as solution we introduced custom InnerReadErrors and InnerWriteErrors.
We moved some methods from the pymethods into the core Rust object and restricted direct access to the inner data, in favor of using public getters/methods.
The SparseObservable docstring is moved to the Python interface for now, though we might want to add a bit more Rust-specific info.

qiskit-bot · 2024-12-23T10:50:39Z

One or more of the following people are relevant to this code:

@Qiskit/terra-core

coveralls · 2024-12-23T11:15:28Z

Pull Request Test Coverage Report for Build 12635149746

Warning: 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.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

1144 of 1204 (95.02%) changed or added relevant lines in 3 files are covered.
34 unchanged lines in 5 files lost coverage.
Overall coverage increased (+0.009%) to 88.96%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
crates/accelerate/src/sparse_observable.rs	1142	1202	95.01%

Files with Coverage Reduction	New Missed Lines	%
crates/accelerate/src/two_qubit_decompose.rs	1	92.13%
crates/accelerate/src/unitary_synthesis.rs	1	92.2%
crates/qasm2/src/lex.rs	2	92.73%
crates/accelerate/src/sparse_observable.rs	10	94.41%
qiskit/quantum_info/operators/symplectic/sparse_pauli_op.py	20	94.58%

Totals
Change from base Build 12420636821:	0.009%
Covered Lines:	79625
Relevant Lines:	89507

💛 - Coveralls

jakelishman

Thanks for doing this.

This is just a quick high-level overview - I'll look more in detail in the new year, especially since I'll have to use a lot more local tools to do a good comparison - with the file move and changes to the code, it's hard to see what's gone on here.

Top level questions:

Why split py_sparse_observable into a separate flat file? I'd have expected any of:
- keep both in the same file
- make a sparse_observable module to put them in
- make a separate crate that contains only the C component
with a rough preference to just keeping everything in the same file for now. This form to me has meant that a lot of logically private functions have had to become pub(crate), and now there's more places to look to understand the code.
For everything that's become pub(crate): in some cases, I think pub(crate) just indicates that a function is defined in the wrong file. In many others, since this PR is looking to a future when SparseObservable is consumable by non-Qiskit crates directly from Rust, I suspect that anything that became pub(crate) should be either private or fully pub. If it's useful for the Python wrapper, feels highly likely it ought to be a proper public interface.

crates/accelerate/src/py_sparse_observable.rs

jakelishman · 2024-12-23T11:38:14Z

crates/accelerate/src/sparse_observable.rs

@@ -146,7 +126,7 @@ impl BitTerm {
    /// returning `Ok(None)` for it.  All other letters outside the alphabet return the complete
    /// error condition.
    #[inline]
-    fn try_from_u8(value: u8) -> Result<Option<Self>, BitTermFromU8Error> {
+    pub(crate) fn try_from_u8(value: u8) -> Result<Option<Self>, BitTermFromU8Error> {


If this needs to be pub beyond this module, I don't see much reason to make it pub(crate) and not just pub, looking to a future.

We might want to make it pub with some of the C functionality, but I wanted to be as cautious as possible -- we can just make it pub pending the structural question you raised above 🙂

Personally I've seen very few reasons for pub(crate) so far - most of them have either been that the function is misplaced, an API boundary has got very muddled, or they should just be pub. We've had quite a lot of churn turning pub(crate) into pub.

3a4b5f0 merged the py_sparse_observable.rs file back into sparse_observable.rs so we don't need to touch the visibility of these methods anymore (though we likely will for the C API later)

crates/accelerate/src/sparse_observable.rs

jakelishman · 2024-12-23T11:43:38Z

crates/accelerate/src/sparse_observable.rs

+    #[inline]
+    pub fn coeffs(&self) -> &Vec<Complex64> {
+        &self.coeffs
+    }
+
+    #[inline]
+    pub fn coeffs_mut(&mut self) -> &mut Vec<Complex64> {
+        &mut self.coeffs
+    }
+
+    #[inline]
+    pub fn indices(&self) -> &Vec<u32> {
+        &self.indices
+    }
+
+    #[inline]
+    pub fn indices_mut(&mut self) -> &mut Vec<u32> {
+        &mut self.indices
+    }
+
+    #[inline]
+    pub fn boundaries(&self) -> &Vec<usize> {
+        &self.boundaries
+    }
+
+    #[inline]
+    pub fn boundaries_mut(&mut self) -> &mut Vec<usize> {
+        &mut self.boundaries
+    }
+
+    #[inline]
+    pub fn bit_terms(&self) -> &Vec<BitTerm> {
+        &self.bit_terms
+    }
+
+    #[inline]
+    pub fn bit_terms_mut(&mut self) -> &mut Vec<BitTerm> {
+        &mut self.bit_terms
+    }


Returning &Vec<T> is almost never correct; you almost invariably mean &[T] (and you do, in every case here). &mut Vec can be correct in some circumstances, but in this case is very not - all these methods allow the Vec-resizing methods to be called, which easily makes the data incoherent. For ones we definitely need to expose, it should be &mut [T].

boundaries_mut and indices_mut might very well be considered unsafe, since you can easily break data coherence by writing bad values to them. I can't remember what the old code did around this, but if they are actually needed and used, then they may well want to be unsafe in Rust interfaces.

Oh right, it should definitely not be Vec here -- I'll fix that 🙂

Regarding necessity of these methods: this mutable access is used in ArrayView::__setitem__, which previously had a SparseObservable as base and simply used mutable access to the internals. But now PySparseObservable it the base and we don't have this access anymore. Using unsafe seems fine, but maybe we should add some validation in __setitem__ 😄

releasenotes/notes/update-sparse-observable-error-2bb4b9e678675eee.yaml

Cryoris · 2024-12-23T13:12:59Z

Thanks for the comments! I think they all make sense but I'll read them more carefully next year as well 🙂 Regarding

Why split py_sparse_observable into a separate flat file? I'd have expected any of:

keep both in the same file

make a sparse_observable module to put them in

make a separate crate that contains only the C components

with a rough preference to just keeping everything in the same file for now. This form to me has meant that a lot of logically private functions have had to become pub(crate), and now there's more places to look to understand the code.

To me, having a separate crate (I assume into py_ext?) sounds the cleanest, but I didn't want just move it w/o discussion, so I moved it into a separate file to facilitate that process 😛 I'm fine with keeping it in the same file as well for now too, though.

jakelishman · 2024-12-23T16:23:36Z

btw should Max be a co-author, or is this all you so far?

- avoid using &Vec - add unsafe to mutable indices/boundaries - rm upgrade reno

Cryoris · 2025-01-06T15:30:30Z

With 3a4b5f0, all code is in a single file for now, but it should be easy to separate if we want to later on (basically the lower half is the Python wrapper)

btw should Max be a co-author, or is this all you so far?

This was my bit so far, since I was reviewing the SparseObservable PRs initially I volunteered to do the split 🙂

Cryoris added 2 commits December 23, 2024 11:15

Split PySparseObservable or SparseObservable

972b06e

reno and docs

2bfdb49

Cryoris added Changelog: API Change Include in the "Changed" section of the changelog Rust This PR or issue is related to Rust code in the repository labels Dec 23, 2024

Cryoris added this to the 2.0.0 milestone Dec 23, 2024

Cryoris requested a review from a team as a code owner December 23, 2024 10:50

jakelishman reviewed Dec 23, 2024

View reviewed changes

Cryoris added 5 commits January 6, 2025 11:54

rm Arc<RwLock> for SparseTerm

fd2f3ee

add Into(Py) for SparseObservable

e4a4db8

rm Rust-side PySparseObservable.clone

c790cff

smaller fixes

4aff73b

- avoid using &Vec - add unsafe to mutable indices/boundaries - rm upgrade reno

move back into 1 file

3a4b5f0

Cryoris removed the Changelog: API Change Include in the "Changed" section of the changelog label Jan 6, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split `PySparseObservable` off `SparseObservable` #13595

Split `PySparseObservable` off `SparseObservable` #13595

Cryoris commented Dec 23, 2024

qiskit-bot commented Dec 23, 2024

coveralls commented Dec 23, 2024 •

edited

Loading

jakelishman left a comment •

edited

Loading

jakelishman Dec 23, 2024

Cryoris Dec 23, 2024

jakelishman Dec 23, 2024

Cryoris Jan 6, 2025

jakelishman Dec 23, 2024 •

edited

Loading

Cryoris Jan 6, 2025 •

edited

Loading

Cryoris commented Dec 23, 2024

jakelishman commented Dec 23, 2024

Cryoris commented Jan 6, 2025

Split PySparseObservable off SparseObservable #13595

Are you sure you want to change the base?

Split PySparseObservable off SparseObservable #13595

Conversation

Cryoris commented Dec 23, 2024

Summary

Details and comments

qiskit-bot commented Dec 23, 2024

coveralls commented Dec 23, 2024 • edited Loading

Pull Request Test Coverage Report for Build 12635149746

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

jakelishman left a comment • edited Loading

Choose a reason for hiding this comment

jakelishman Dec 23, 2024

Choose a reason for hiding this comment

Cryoris Dec 23, 2024

Choose a reason for hiding this comment

jakelishman Dec 23, 2024

Choose a reason for hiding this comment

Cryoris Jan 6, 2025

Choose a reason for hiding this comment

jakelishman Dec 23, 2024 • edited Loading

Choose a reason for hiding this comment

Cryoris Jan 6, 2025 • edited Loading

Choose a reason for hiding this comment

Cryoris commented Dec 23, 2024

jakelishman commented Dec 23, 2024

Cryoris commented Jan 6, 2025

Split `PySparseObservable` off `SparseObservable` #13595

Split `PySparseObservable` off `SparseObservable` #13595

coveralls commented Dec 23, 2024 •

edited

Loading

jakelishman left a comment •

edited

Loading

jakelishman Dec 23, 2024 •

edited

Loading

Cryoris Jan 6, 2025 •

edited

Loading