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

IQP as circuit library function (and in Rust) #13241

Merged
merged 12 commits into from
Oct 30, 2024

Conversation

Cryoris
Copy link
Contributor

@Cryoris Cryoris commented Sep 30, 2024

Summary

Part of #13046. Moves IQP (instantaneous quantum polytime) circuits to be a function, and ports the internals to Rust.

Details and comments

  • This also adds the feature of creating random IQP circuits by just setting the number of qubits, without explicitly requiring the interactions matrix.
  • The speedup from porting to Rust is more noticeable for large circuits (100+ qubits), where it is ~55x faster. See the table below for more numbers 🙂
IQP timings
-- n = 10, speedup 4.48x
main: 0.001 +- 0.000
this: 0.000 +- 0.000

-- n = 20, speedup 12.59x
main: 0.002 +- 0.000
this: 0.000 +- 0.000

-- n = 50, speedup 45.07x
main: 0.009 +- 0.000
this: 0.000 +- 0.000

-- n = 100, speedup 58.98x
main: 0.034 +- 0.002
this: 0.001 +- 0.000

-- n = 200, speedup 58.45x
main: 0.133 +- 0.004
this: 0.002 +- 0.001

-- n = 500, speedup 51.41x
main: 0.862 +- 0.037
this: 0.017 +- 0.010

@Cryoris Cryoris added 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 Sep 30, 2024
@Cryoris Cryoris added this to the 1.3.0 milestone Sep 30, 2024
@Cryoris Cryoris requested a review from a team as a code owner September 30, 2024 11:10
@qiskit-bot
Copy link
Collaborator

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

  • @Cryoris
  • @Qiskit/terra-core
  • @ajavadia

@coveralls
Copy link

coveralls commented Sep 30, 2024

Pull Request Test Coverage Report for Build 11590872936

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

  • 111 of 114 (97.37%) changed or added relevant lines in 5 files are covered.
  • 104 unchanged lines in 7 files lost coverage.
  • Overall coverage decreased (-0.003%) to 88.699%

Changes Missing Coverage Covered Lines Changed/Added Lines %
qiskit/circuit/library/iqp.py 20 21 95.24%
crates/accelerate/src/circuit_library/iqp.rs 87 89 97.75%
Files with Coverage Reduction New Missed Lines %
crates/accelerate/src/two_qubit_decompose.rs 1 92.09%
qiskit/transpiler/passes/synthesis/unitary_synthesis.py 2 58.05%
crates/qasm2/src/lex.rs 4 92.48%
crates/qasm2/src/parse.rs 6 97.62%
qiskit/synthesis/two_qubit/xx_decompose/decomposer.py 7 90.77%
qiskit/quantum_info/operators/symplectic/base_pauli.py 30 88.55%
crates/accelerate/src/sparse_observable.rs 54 95.42%
Totals Coverage Status
Change from base Build 11584707253: -0.003%
Covered Lines: 75504
Relevant Lines: 85124

💛 - Coveralls

Copy link
Member

@ShellyGarion ShellyGarion left a comment

Choose a reason for hiding this comment

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

Looks good. I only had a few questions.

crates/accelerate/src/circuit_library/iqp.rs Show resolved Hide resolved
crates/accelerate/src/circuit_library/iqp.rs Show resolved Hide resolved
# otherwise validate the interactions
else:
if num_qubits is not None:
raise ValueError("Only one of interactions or num_qubits can be provided, not both.")
Copy link
Member

Choose a reason for hiding this comment

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

perhaps both could be given, and one can assert that num_qubits = len(interactions) in this case ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I thought about that, it seemed a bit strange that passing an additional argument could raise an error in a function when one could just omit it. Explicitly disallowing passing both would make sure users can only have a single way of running the code... but I'm also happy to allow num_qubits 🙂

Copy link
Contributor

@alexanderivrii alexanderivrii 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 looks great. One question is why we need the new "random IQP" mode, is this used anywhere in Qiskit? In addition, personally I would prefer to have a separate function that returns a random IQP circuit.

qiskit/circuit/library/iqp.py Outdated Show resolved Hide resolved
crates/accelerate/src/circuit_library/iqp.rs Outdated Show resolved Hide resolved
crates/accelerate/src/circuit_library/iqp.rs Outdated Show resolved Hide resolved
crates/accelerate/src/circuit_library/iqp.rs Outdated Show resolved Hide resolved
crates/accelerate/src/circuit_library/iqp.rs Show resolved Hide resolved
@Cryoris
Copy link
Contributor Author

Cryoris commented Oct 25, 2024

As discussed offline I split the iqp function into iqp and random_iqp to have a separate function for the random generation of IQP circuits 🙂

i hope this was the cause of the error, lets see
Copy link
Contributor

@alexanderivrii alexanderivrii left a comment

Choose a reason for hiding this comment

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

This looks good. Just one comment about generating random IQP instances.

crates/accelerate/src/circuit_library/iqp.rs Outdated Show resolved Hide resolved
Comment on lines +23 to +24
m.add_wrapped(wrap_pyfunction!(iqp::py_iqp))?;
m.add_wrapped(wrap_pyfunction!(iqp::py_random_iqp))?;
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think it matters too much, but this uses a slightly different naming convention from most of the other circuit library code (i.e. here the functions are prefixed with py).

-- the diagonal entries on the diagonal got lost!

Co-authored-by: Alexander Ivrii <alexi@il.ibm.com>
Copy link
Contributor

@alexanderivrii alexanderivrii 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 the quick fix, LGTM!

@alexanderivrii alexanderivrii added this pull request to the merge queue Oct 30, 2024
Merged via the queue into Qiskit:main with commit 91ceee5 Oct 30, 2024
17 checks passed
@ShellyGarion ShellyGarion added the Changelog: New Feature Include in the "Added" section of the 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: New Feature Include in the "Added" section of the changelog mod: circuit Related to the core of the `QuantumCircuit` class or the circuit library 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.

5 participants