Split PySparseObservable off SparseObservable

[Cryoris]

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]

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

• @Qiskit/terra-core

[coveralls]

Pull Request Test Coverage Report for Build 12992269684

Details

• 1136 of 1205 (94.27%) changed or added relevant lines in 1 file are covered.
• 27 unchanged lines in 5 files lost coverage.
• Overall coverage decreased (-0.02%) to 88.915%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
crates/accelerate/src/sparse_observable.rs	1136	1205	94.27%

Files with Coverage Reduction	New Missed Lines	%
crates/accelerate/src/unitary_synthesis.rs	1	92.97%
crates/qasm2/src/expr.rs	1	94.23%
crates/accelerate/src/sparse_observable.rs	2	94.34%
crates/qasm2/src/lex.rs	5	91.98%
crates/qasm2/src/parse.rs	18	96.22%

Totals
Change from base Build 12990725350:	-0.02%
Covered Lines:	79690
Relevant Lines:	89625

---

💛 - 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.

[Cryoris]

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]

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

[Cryoris]

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 🙂

[jakelishman]

This looks like a really nice, clean split, thank you!

I pushed up one minor stylistic commit (just moving trait impls closer to the relevant structs, except when doing so would move "Python" code into "Rust" code), and I sorted out the merge conflict by upgrading the PySparseObservable conversions into the new PyO3 form. Feel free to mess with those two commits if I've done something against what you wanted.

I had one non-actionable comment and one pretty minor nitpick, but other than that I'm very happy with this and ready to merge!

[jakelishman]

Thanks for all the work on this!

[jakelishman]

Technically this was the only line that actually needed to chance to implement my query (and then, just the string name), but giving them the same name is better anyway.

At some point in the near-ish future when I've lost my rage at my initial failed attempt, I'll post a PR that turns the submodule-construction logic back into more native PyO3 tooling, since PyO3 0.23 gave us the APIs we need, and abi3audit removed the failure we were getting from it anyway (for reasons I don't 100% agree with).

[jakelishman]

Wait, no - one second, I think your last merge undid a bunch of stuff.

[jakelishman]

I straightened out the history to remove the second merge, but actually I think I was mistaken anyway - I think I just confused myself because the merge commit you pushed up had the same diff as the one I'd made (since those were the changes you were merging).

[Cryoris]

Thanks to you for the help! I'm very happy to see this merged and move forward with the C API bit 😁