Skip to content

Conversation

velocityraptor7085
Copy link
Contributor

@velocityraptor7085 velocityraptor7085 commented Jun 2, 2025

Summary

Details and comments

The changes made in the rust-based implementation include:

  1. Adding a dependency on the qiskit-quantum-info crate the Cargo.toml file within the qiskit-accelerate crate.
  2. Adding a rust function (sampled_expval_sparse_observable) to extract the operator strings and coefficients (instead of in Python), and pass them into the existing rust routines for expectation value computations (in crates/accelerate/src/sampled_exp_val.rs).
  3. Makes the inner trait of the PySparseObservable wrapper (in crates/quantum_info/src/sparse_observable/mod.rs) as pub (This was done since passing a PyRef<SparseObservable needed a PyClass implementation, but adding #[pyclass] to the SparseObservable rust struct causes conflicting implementations due to the wrapper's implementation. I am unsure if there is a more elegant / desirable way to do this and would appreciate any comments on the same).
  4. Calls the rust routine from the sampled_expval.py in an additional elif clause.

@velocityraptor7085 velocityraptor7085 requested a review from a team as a code owner June 2, 2025 07:19
@qiskit-bot qiskit-bot added the Community PR PRs from contributors that are not 'members' of the Qiskit repo label Jun 2, 2025
@qiskit-bot
Copy link
Collaborator

Thank you for opening a new pull request.

Before your PR can be merged it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient.

While you're waiting, please feel free to review other open PRs. While only a subset of people are authorized to approve pull requests for merging, everyone is encouraged to review open pull requests. Doing reviews helps reduce the burden on the core team and helps make the project's code better for everyone.

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

  • @Qiskit/terra-core

ghost

This comment was marked as spam.

@coveralls
Copy link

coveralls commented Jun 2, 2025

Pull Request Test Coverage Report for Build 17955752966

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.

Details

  • 52 of 59 (88.14%) changed or added relevant lines in 3 files are covered.
  • 1240 unchanged lines in 29 files lost coverage.
  • Overall coverage decreased (-0.07%) to 88.307%

Changes Missing Coverage Covered Lines Changed/Added Lines %
crates/accelerate/src/sampled_exp_val.rs 44 51 86.27%
Files with Coverage Reduction New Missed Lines %
crates/cext/src/transpiler/passes/split_2q_unitaries.rs 1 90.63%
crates/cext/src/transpiler/passes/vf2.rs 1 82.61%
crates/circuit/src/parameter/parameter_expression.rs 1 82.79%
crates/qasm2/src/expr.rs 1 93.63%
crates/quantum_info/src/sparse_observable/mod.rs 1 94.04%
crates/qasm2/src/lex.rs 2 92.27%
qiskit/transpiler/passes/layout/sabre_pre_layout.py 2 96.55%
crates/cext/src/transpiler/passes/unitary_synthesis.rs 3 96.67%
crates/quantum_info/src/unitary_sim.rs 4 85.11%
crates/quantum_info/src/convert_2q_block_matrix.rs 6 92.75%
Totals Coverage Status
Change from base Build 17500557721: -0.07%
Covered Lines: 92787
Relevant Lines: 105073

💛 - Coveralls

Copy link
Member

@mtreinish mtreinish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good start, thanks for looking at this in Rust this is a big improvement over doing it in Python. I have a few suggestions on how to make the code more efficient inline. Besides those suggestions I think the biggest issue is that we shouldn't be accessing the inner sparse observable object public, the public api of the sparse observable should give us the tools to get the data we need already, and if it doesn't we should expand the api with methods to get the data as appropriate.

@kaldag kaldag requested review from mtreinish and Cryoris September 8, 2025 04:22
Copy link
Member

@mtreinish mtreinish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking this over, I think this is pretty close, I just had a couple inline comments.

@kaldag kaldag requested a review from Cryoris September 23, 2025 18:45
@kaldag kaldag requested a review from Cryoris September 24, 2025 18:39
Copy link
Contributor

@Cryoris Cryoris left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM thanks for the updates!

@Cryoris Cryoris added this pull request to the merge queue Sep 25, 2025
Merged via the queue into Qiskit:main with commit c34d75b Sep 25, 2025
26 checks passed
@jakelishman jakelishman added the Changelog: New Feature Include in the "Added" section of the changelog label Oct 7, 2025
@jakelishman jakelishman added this to the 2.3.0 milestone Oct 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: New Feature Include in the "Added" section of the changelog Community PR PRs from contributors that are not 'members' of the Qiskit repo
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

sampled_expectation_value does not accept SparseObservable
7 participants