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 a capacity-allocated constructor for DAGCircuit in Rust. #12975

Merged
merged 4 commits into from
Sep 3, 2024

Conversation

raynelfss
Copy link
Contributor

@raynelfss raynelfss commented Aug 18, 2024

Summary

Resolves #13002
After #12550 merges, the following commits aim to provide awith_capacity alternative initializer for the DAGCircuit in Rust.

Details and comments

Will be rebased after #12550 merges

These commits add the DAGCircuit::with_capacity constructor to the rust representation of DAGCircuit. This would allow us to allocate an approximation of the necessary space to successfully create a DAGCircuit with certain properties to avoid re-allocating space in memory.

This constructor takes the following arguments:

  • num_qubits for the number of qubits in the specific circuit.
  • num_clbits for the number of clbits in the circuit.
  • num_ops for the number of operations in the circuit (Optional).
  • num_vars for the number of variables in the circuit (Optional).

Questions

  • What is the best way of estimating the possible number of edges required for num_ops?
    • An operation (or a block) can have as many input/output qargs/cargs as possible
    • I used 3 (2 qubits + 1 classical or 3 qubits) as a baseline. But this is just speculation, if a better approach exists, let me in a comment.
  • Best way of estimating the names of operations in a circuit?
    • In a DAGCircuit, there can be 100 op_nodes of just the same operation or 100 different operations occurring just once.
    • I initialized it as num_ops but if a different opinion exists, let me know in a comment.

Blockers

@raynelfss raynelfss added on hold Can not fix yet Rust This PR or issue is related to Rust code in the repository mod: circuit Related to the core of the `QuantumCircuit` class or the circuit library labels Aug 18, 2024
@coveralls
Copy link

coveralls commented Aug 18, 2024

Pull Request Test Coverage Report for Build 10690338962

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

  • 72 of 72 (100.0%) changed or added relevant lines in 3 files are covered.
  • 169 unchanged lines in 13 files lost coverage.
  • Overall coverage increased (+0.03%) to 89.171%

Files with Coverage Reduction New Missed Lines %
qiskit/circuit/commutation_checker.py 1 94.44%
qiskit/synthesis/two_qubit/xx_decompose/decomposer.py 1 95.42%
qiskit/quantum_info/operators/symplectic/random.py 2 95.0%
qiskit/transpiler/passes/optimization/commutative_inverse_cancellation.py 2 96.92%
crates/qasm2/src/lex.rs 4 91.98%
crates/circuit/src/packed_instruction.rs 4 95.19%
crates/accelerate/src/synthesis/linear/utils.rs 8 94.83%
qiskit/circuit/library/data_preparation/pauli_feature_map.py 9 91.0%
crates/circuit/src/dag_circuit.rs 9 88.87%
crates/qasm2/src/parse.rs 12 96.69%
Totals Coverage Status
Change from base Build 10638423704: 0.03%
Covered Lines: 72524
Relevant Lines: 81331

💛 - Coveralls

@raynelfss raynelfss force-pushed the rusty-dag-with-capacity branch from 2533f4b to 66c3c0e Compare August 19, 2024 17:28
@mtreinish
Copy link
Member

#12812 might be useful for you on this. This was me adding basically the same interface to the python space constructor for DAGCircuit. We get more flexibility with it in Rust including for things like the internal caches so I probably will rebase that PR on top of this after this merges (and add a python interface for the alternative constructor).

@qiskit-bot
Copy link
Collaborator

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

  • @Qiskit/terra-core
  • @kevinhartman
  • @mtreinish

@raynelfss raynelfss changed the title [WIP] Implement a capacity-allocated constructor for DAGCircuit in Rust. Implement a capacity-allocated constructor for DAGCircuit in Rust. Aug 26, 2024
@raynelfss raynelfss mentioned this pull request Aug 26, 2024
6 tasks
@raynelfss raynelfss removed the on hold Can not fix yet label Aug 26, 2024
@raynelfss raynelfss force-pushed the rusty-dag-with-capacity branch from c86aa6b to 9b9f124 Compare August 26, 2024 18:58
- Implement `DAGCircuit` with `with_capacity` to create an initially allocated instance, for rust only.
- Implement `with_capacity` for `BitData` and `IndexInterner` that creates an initally allocated instance of each type.
- Other small tweaks and fixes.
- Add num_edges optional argument. If known, use `num_edges` argument to create a `DAGCircuit` with the exact number of edges.
- Leverage usage of new method in `copy_empty_like`.
- Use `..Default::default()` for `op_names` and `calibrations` fields in `DAGCircuit::with_capacity()`
@raynelfss raynelfss force-pushed the rusty-dag-with-capacity branch from 9b9f124 to 4d12377 Compare August 27, 2024 19:01
@raynelfss raynelfss added this to the 1.3 beta milestone Sep 3, 2024
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.

Overall this LGTM it's a straightforward addition to the rust api for DAGCircuit. Just one inline comment where I think one argument is in the wrong position.

crates/circuit/src/dag_circuit.rs Show resolved Hide resolved
- The order now follows: qubits, clbits, vars, num_ops, edges. As per [Matthew's comment](Qiskit#12975 (comment)).
@raynelfss raynelfss requested a review from mtreinish September 3, 2024 20:44
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.

LGTM, thanks for the quick update

@mtreinish mtreinish enabled auto-merge September 3, 2024 21:28
@mtreinish mtreinish added this pull request to the merge queue Sep 3, 2024
Merged via the queue into Qiskit:main with commit dff9e81 Sep 3, 2024
15 checks passed
sbrandhsn pushed a commit to sbrandhsn/qiskit that referenced this pull request Sep 5, 2024
…iskit#12975)

* Initial: implement fixed capacity constructor for DAGCircuit
- Implement `DAGCircuit` with `with_capacity` to create an initially allocated instance, for rust only.
- Implement `with_capacity` for `BitData` and `IndexInterner` that creates an initally allocated instance of each type.
- Other small tweaks and fixes.
- Add num_edges optional argument. If known, use `num_edges` argument to create a `DAGCircuit` with the exact number of edges.
- Leverage usage of new method in `copy_empty_like`.
- Use `..Default::default()` for `op_names` and `calibrations` fields in `DAGCircuit::with_capacity()`

* Fix: Adapt to Qiskit#13033

* Fix: Re-arrange the function arguments as per @mtreinish's review.
- The order now follows: qubits, clbits, vars, num_ops, edges. As per [Matthew's comment](Qiskit#12975 (comment)).
@raynelfss raynelfss added the Changelog: None Do not include in changelog label Nov 5, 2024
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 mod: circuit Related to the core of the `QuantumCircuit` class or the circuit library priority: high 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.

Create a DAGCircuit::with_capacity method in Rust
4 participants