Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement num_qubits and num_clbits in Rust #12495

Merged
merged 3 commits into from
Jun 4, 2024

Conversation

jlapeyre
Copy link
Contributor

@jlapeyre jlapeyre commented Jun 3, 2024

  • QuantumCircuit.num_qubits and num_clbits are twice as fast after this PR.
  • These are used in several places. For example QuantumCircuit.width() is three times faster.
  • num_qubits and num_clbits are introduced in circuit_data.rs. These functions are called by the corresponding Python methods. They are also used in circuit_data.rs itself.

* QuantumCircuit.num_qubits and num_clbits are twice as fast after
  this PR.
* These are used in several places. For example QuantumCircuit.width()
  is three times faster.
* num_qubits and num_clbits are introduced in circuit_data.rs. These
  functions are called by the corresponding Python methods.
  They are also used in circuit_data.rs itself.
@jlapeyre jlapeyre marked this pull request as ready for review June 3, 2024 16:04
@jlapeyre jlapeyre requested a review from a team as a code owner June 3, 2024 16:04
@qiskit-bot
Copy link
Collaborator

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

@coveralls
Copy link

coveralls commented Jun 3, 2024

Pull Request Test Coverage Report for Build 9371133565

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

  • 29 of 29 (100.0%) changed or added relevant lines in 2 files are covered.
  • 19 unchanged lines in 3 files lost coverage.
  • Overall coverage decreased (-0.008%) to 89.59%

Files with Coverage Reduction New Missed Lines %
crates/qasm2/src/lex.rs 2 93.38%
qiskit/circuit/parameterexpression.py 5 96.51%
crates/qasm2/src/parse.rs 12 97.15%
Totals Coverage Status
Change from base Build 9327235113: -0.008%
Covered Lines: 62367
Relevant Lines: 69614

💛 - 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 LGTM, in general as the source of truth moves more to the rust side I think this is fine.

There is an open question about a rust native representation of bits after #12459 but in the meantime asserting the bit pyobject vec in the circuit data is the source of truth seems fine to me (which this PR is doing implicitly), especially once we move to the gate representation being owned by the rust domain.

@@ -220,6 +220,11 @@ impl CircuitData {
self.qubits.clone_ref(py)
}

#[getter]
Copy link
Member

Choose a reason for hiding this comment

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

Do you want to throw a docstring on this and the other new methods just for completeness? While we don't publish docs for this as a pyclass (since it's internal only), it sometimes is a nice hint for people using IDEs that show the __doc__ attribute of a python class.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh yeah. I forgot to add some of this when I moved it out of draft.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 2ffac76

@jlapeyre
Copy link
Contributor Author

jlapeyre commented Jun 4, 2024

asserting the bit pyobject vec in the circuit data is the source of truth seems fine to me

It wasn't 100% clear to me that qubits_native: Vec<PyObject> is already the fundamental object. But the word "cached" here

/// The qubits registered, cached as a ``list[Qubit]``.
qubits: Py<PyList>,

made it seem more likely.

an open question about a rust native representation of bits

Unfortunately, I expect there will be a lot of churn like this is gradually moving things to Rust. But this PR is pretty light in terms of changed code, so not too onerous to replace.

@mtreinish
Copy link
Member

It wasn't 100% clear to me that qubits_native: Vec is already the fundamental object. But the word "cached" here

My mistake tou're right it already is, I was misremembering the state of the code. The Python side QuantumCircuit.qubits is returning the inner QuantumCircuit._data.qubits already. So there is no concern at all with doing this via rust for improved efficiency.

self.clbits_native.len()
}

pub fn width(&self) -> usize {
Copy link
Member

Choose a reason for hiding this comment

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

This is missing a docstring too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done here 16755cd

@mtreinish mtreinish added performance Changelog: None Do not include in changelog Rust This PR or issue is related to Rust code in the repository labels Jun 4, 2024
@mtreinish mtreinish added this pull request to the merge queue Jun 4, 2024
Merged via the queue into Qiskit:main with commit 72ff2cf Jun 4, 2024
15 checks passed
@jlapeyre jlapeyre deleted the num-qubits-rust branch June 5, 2024 15:42
jlapeyre added a commit to jlapeyre/qiskit-core that referenced this pull request Jun 5, 2024
* Implement num_qubits and num_clbits in Rust

* QuantumCircuit.num_qubits and num_clbits are twice as fast after
  this PR.
* These are used in several places. For example QuantumCircuit.width()
  is three times faster.
* num_qubits and num_clbits are introduced in circuit_data.rs. These
  functions are called by the corresponding Python methods.
  They are also used in circuit_data.rs itself.

* Add doc strings for rust implemented num_qubits and num_clbits

* Add doc string for `width` in Rust
Procatv pushed a commit to Procatv/qiskit-terra-catherines that referenced this pull request Aug 1, 2024
* Implement num_qubits and num_clbits in Rust

* QuantumCircuit.num_qubits and num_clbits are twice as fast after
  this PR.
* These are used in several places. For example QuantumCircuit.width()
  is three times faster.
* num_qubits and num_clbits are introduced in circuit_data.rs. These
  functions are called by the corresponding Python methods.
  They are also used in circuit_data.rs itself.

* Add doc strings for rust implemented num_qubits and num_clbits

* Add doc string for `width` in Rust
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: None Do not include in changelog performance Rust This PR or issue is related to Rust code in the repository
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants