Use Rust generator function for QuantumVolume class #13283
Conversation
In Qiskit#13238 we added a new function quantum_volume for generating a quantum volume model circuit with the eventual goal of replacing the existing QuantumVolume class. This new function is ~10x faster to generate the circuit than the existing python class. This commit builds off of that to internally call the new function for generating the circuit from the class. While the plan is to deprecate and remove the class for Qiskit 2.0 until that time we can give a performance boost to users of it for the lifespan of the 1.x release series.
mtreinish
added
performance
Changelog: New Feature
|
One or more of the following people are relevant to this code:
|
| @@ -83,39 +83,48 @@ def __init__( | |||
| depth = depth or num_qubits # how many layers of SU(4) | |||
| width = num_qubits // 2 # how many SU(4)s fit in each layer | |||
| rng = seed if isinstance(seed, np.random.Generator) else np.random.default_rng(seed) | |||
| seed_name = seed | |||
| if seed is None: | |||
There was a problem hiding this comment.
IMO it would be clearer to check if seed_name is None instead, since the purpose of this branch is to make sure it has a value.
| else: | ||
| self._append(CircuitInstruction(qv_circ.to_instruction(), tuple(self.qubits))) |
There was a problem hiding this comment.
Should we instead just early-return here instead of having the else on line 107? That way, it's a bit clearer that the handling of classical permutations is treated specially and forwarded to the Rust quantum_volume.
There was a problem hiding this comment.
I didn't return because __init__ normally doesn't have a return; that's an implicit None but there is a pylint rule that doesn't like an explicit return in one path and not another.
There was a problem hiding this comment.
I think pylint is fine with bare return for early return, it's return None it has a problem with (unless it's changed).
There was a problem hiding this comment.
Ah, that's probably it, I normally would do return None in this case. I can change it to a bare return if there is a strong preference here.
|
I did a quick speed test of a 100q qv model circuit, with |
Pull Request Test Coverage Report for Build 11243570672Details
💛 - Coveralls |
| CircuitInstruction(gate, (qubits[perm[qubit]], qubits[perm[qubit + 1]])) | ||
| ) | ||
| if classical_permutation: | ||
| if seed is not None: |
There was a problem hiding this comment.
I might be slower than usual as I'm reviewing this before my first coffee, but should this be the following?
| if seed is not None: | |
| if seed is None: |
There was a problem hiding this comment.
This is actually correct. This code path is to normalize the seeds we pass to rust so it will fit in a uint64 (although I realized I used int64 by mistake, I'll fix that). The typing on the python class was a python int which was doesn't have an upper bound and it was possible to pass in an value greater than 18446744073709551615 which would cause an error when one didn't happen before. To solve that I used a numpy rng to select a random int from the provided seed value that we then pass to rust as a seed for it's internal rng.
When seed is None we can just pass that to rust because it will use system entropy to initialize the rust rng.
Summary
In #13238 we added a new function quantum_volume for generating a quantum volume model circuit with the eventual goal of replacing the existing QuantumVolume class. This new function is ~10x faster to generate the circuit than the existing python class. This commit builds off of that to internally call the new function for generating the circuit from the class. While the plan is to deprecate and remove the class for Qiskit 2.0 until that time we can give a performance boost to users of it for the lifespan of the 1.x release series.
Details and comments