-
Notifications
You must be signed in to change notification settings - Fork 55
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
RBDesign bugfixes and improvements #443
Conversation
Allows optional return of number of native gates per Clifford during random circuit compilation, and stores these in the CliffordRBDesign. Adds utility function for computing average native gates per Clifford, and ensures this works through design truncation. Also adds a missing utility to compute total number of gates to Circuits.
Generalizes serialization/truncation for attributes that are "paired" with the circuit lists in BenchmarkingDesigns. This removes the code duplication in inherited classes with more "paired" attributes, such as CliffordRBDesign (with the new native gate info) and BinaryRBDesign (with measurements/signs).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly looks good. I made one suggestion to factor out some logic into its own function, and left one comment about a deepcopy.
pygsti/algorithms/randomcircuit.py
Outdated
for _ in range(0, length + 1): | ||
if exact_compilation_key is not None: | ||
# Deterministic compilation based on a provided clifford compilation | ||
assert exact_compilation_key in clifford_compilations, \ | ||
f"{exact_compilation_key} not provided in `clifford_compilations`" | ||
|
||
# Pick clifford | ||
cidx = rand_state.randint(24**n) | ||
lbl = _lbl.Label(f'C{cidx}', qubits) | ||
|
||
# Try to do deterministic compilation | ||
try: | ||
circuit = clifford_compilations[exact_compilation_key].retrieve_compilation_of(lbl) | ||
except AssertionError: | ||
raise ValueError( | ||
f"Failed to compile n-qubit Clifford 'C{cidx}'. Ensure this is provided in the " + \ | ||
"compilation rules, or use a compilation algorithm to synthesize it by not " + \ | ||
"specifying `exact_compilation_key`." | ||
) | ||
|
||
# compute the symplectic rep of the chosen clifford | ||
s, p = _symp.symplectic_rep_of_clifford_circuit(circuit, srep_cache) | ||
else: | ||
# Random compilation | ||
s, p = _symp.random_clifford(n, rand_state=rand_state) | ||
circuit = _cmpl.compile_clifford(s, p, pspec, | ||
clifford_compilations.get('absolute', None), | ||
clifford_compilations.get('paulieq', None), | ||
qubit_labels=qubit_labels, iterations=citerations, *compilerargs, | ||
rand_state=rand_state) | ||
num_native_gates += circuit.num_gates |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can the contents of this loop be factored out into a new function? It looks semantically meaningful, and this create_clifford_rb_circuit
function is quite long.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, will refactor that function out!
def truncate_to_lists(self, list_indices_to_keep): | ||
""" | ||
Truncates this experiment design by only keeping a subset of its circuit lists. | ||
|
||
Parameters | ||
---------- | ||
list_indices_to_keep : iterable | ||
A list of the (integer) list indices to keep. | ||
|
||
Returns | ||
------- | ||
ByDepthDesign | ||
The truncated experiment design. | ||
""" | ||
ret = _copy.deepcopy(self) # Works for derived classes too | ||
ret.depths = [self.depths[i] for i in list_indices_to_keep] | ||
ret.circuit_lists = [self.circuit_lists[i] for i in list_indices_to_keep] | ||
return ret |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might there be scenarios where the deepcopy isn't needed? If so, it might be good to have an optional function argument deepcopy=False
that controls whether the copy is deep or shallow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There could be cases where the deepcopy is not desired - for example, if we want to create a sort of "nested" edesign and want to point to the original circuit lists.
However, I'm inclined to keep this deepcopy as-is for now because it's actually indicative of a hack to make these functions work for derived classes. I think we could more cleanly do this with Generics/TypeVars, which I've been learning more about recently, but I want to keep this the same as the pattern used elsewhere in the code so I can clean it up all at once later and don't feel like plumbing a temporary kwarg in through all of this now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This all looks good to me. Once we get the seal of approval from one of the RB codeowners I think this is good to go.
pygsti/algorithms/randomcircuit.py
Outdated
for _ in range(0, length + 1): | ||
if exact_compilation_key is not None: | ||
# Deterministic compilation based on a provided clifford compilation | ||
assert exact_compilation_key in clifford_compilations, \ | ||
f"{exact_compilation_key} not provided in `clifford_compilations`" | ||
|
||
# Pick clifford | ||
cidx = rand_state.randint(24**n) | ||
lbl = _lbl.Label(f'C{cidx}', qubits) | ||
|
||
# Try to do deterministic compilation | ||
try: | ||
circuit = clifford_compilations[exact_compilation_key].retrieve_compilation_of(lbl) | ||
except AssertionError: | ||
raise ValueError( | ||
f"Failed to compile n-qubit Clifford 'C{cidx}'. Ensure this is provided in the " + \ | ||
"compilation rules, or use a compilation algorithm to synthesize it by not " + \ | ||
"specifying `exact_compilation_key`." | ||
) | ||
|
||
# compute the symplectic rep of the chosen clifford | ||
s, p = _symp.symplectic_rep_of_clifford_circuit(circuit, srep_cache) | ||
else: | ||
# Random compilation | ||
s, p = _symp.random_clifford(n, rand_state=rand_state) | ||
circuit = _cmpl.compile_clifford(s, p, pspec, | ||
clifford_compilations.get('absolute', None), | ||
clifford_compilations.get('paulieq', None), | ||
qubit_labels=qubit_labels, iterations=citerations, *compilerargs, | ||
rand_state=rand_state) | ||
num_native_gates += circuit.num_gates |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
pygsti/algorithms/randomcircuit.py
Outdated
f"{exact_compilation_key} not provided in `clifford_compilations`" | ||
|
||
# Pick clifford | ||
cidx = rand_state.randint(24**n) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is incorrect because there are not 24**n Cliffords. You can use tools.sympletic.compute_num_cliffords(n) to get the number of Cliffords
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch!
pygsti/algorithms/randomcircuit.py
Outdated
) | ||
|
||
# compute the symplectic rep of the chosen clifford | ||
s, p = _symp.symplectic_rep_of_clifford_circuit(circuit, srep_cache) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note for if we ever want this to run quickly (we probably don't care) -- This is slightly absurdly way to do this, as given the index of the Clifford we should be able to easily compute s and p directly. However, there's only code right now to easily compute s (compute_symplectic_matrix(i, n)) and not p so there's no to-do I'm suggesting here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed that this is not the best way to do this. What I actually wanted to do (and might still be useful in the future) was reuse the generate s/p code and then be able to look up the rule for compiling that Clifford from the CompilationRules. The CliffordCompRule object has some support for symplectic reps already, although not this reverse lookup (and I didn't want to restrict this to only taking CliffordCompRules).
So either way, I think we can revisit this when we update the symplectic tools to have either functionality (and I've left a TODO comment in the code noting that).
return sum([cnt(sub) for sub in obj]) | ||
|
||
return sum([cnt(layer_lbl) for layer_lbl in self._labels]) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understand this correctly, this is computing the number of explicitly included (i.e., not implicit idle) gates in the circuit? I.e., this is different to self.depth * self.width right? If so, (1) this seems like a useful function to have, and (2) it's debatable whether this is what should be computed by the RB code. I suspect that what should be returned is this and the circuit size (width * depth)? Some users might also be interested in the 2-qubit gate count, so that could also be computed and all wrapped up in a dictionary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are correct, this is explicitly included gates and not depth*width (which already exists as circuit.size
). FYI, the 2-qubit gate count also exists in that you can do circuit.num_nq_gates(2)
.
Computing all three and wrapping it in a dict is a pretty easy and self-documenting solution, so I'll implement that. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pygsti/protocols/rb.py
Outdated
num_clifford_gates = 0 | ||
for list_idx in range(len(self.depths)): | ||
num_native_gates = sum(self.num_native_gate_lists[list_idx]) | ||
num_clifford_gates = len(self.num_native_gate_lists[list_idx]) * (self.depths[list_idx] + 1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should these two lines be a += not a =?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch, that's a copy-paste error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is one error (the size of the Clifford group) that needs to be fixed. The bit about adding different statistics about the native gates in each Clifford is optional, and an alternative to making the suggested additions is to edit the doc-string to make it very clear what the number of native gates per Clifford means here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! :)
Fixes several RB issues. I still plan to write a few tests for these, but it is at least ready to review from the RB codeowners to ensure that these are good solutions to the features.
Deterministic Clifford compilation (#314)
The desire was to be able to do deterministic Clifford compilation when generating
CliffordRBDesign
objects. We actually already had a partial way to do this because theCliffordCompilationRules
are more or less exactly this - a rule for how to build up a circuit from native gates that implement a Clifford. So my implementation is just to add aexact_compilation_key
kwarg which uses the compilation rules to do this deterministic compilation when provided, and otherwise to do the previous behavior of synthesizing using the random algorithms.An example:
This will create the design as before, with random Clifford synthesis:
This will create a design using the
absolute
compilation of each Clifford instead:Or similarly, we could do a compilation up to Pauli equivalence with the
paulieq
compilation:A non-standard compilation rule could also be done:
Native gate per Clifford statistics (#315)
The
CliffordRBDesign
now keeps track of how many native gates are in the firstlength
+1 Cliffords, i.e. excluding the inversion Clifford, stores them in anative_gate_count_lists
member variable, and provides convenience functions to compute the averages.As an aside, I also added a
num_gates
utility function toCircuit
that was apparently missing. There wasnum_nq_gates
andnum_multiq_gates
so you could have donenum_nq_gates(1) + num_multiq_gates
, but might as well provide it.Truncation bugfix for RBDesigns (#408)
The various
BenchmarkingDesign
classes often have lists that are intended to be paired with thecircuit_lists
member attribute. These paired attributes were not being truncated properly.To fix this, the
BenchmarkingDesign
now has apaired_with_circuit_attrs
member variable which lists the names of any attribute that is intended to correspond 1-to-1 with thecircuit_lists
. During truncation, these paired attributes are zipped up with the circuits during filtering, and then unzipped into the new truncated lists. Paired attributes are also serialized separately to JSON files, as we often want to look at them independently.A currently exhaustive list of paired attributes (paired attributes propogate to derived classes):
BenchmarkingDesign
:idealout_lists
CliffordRBDesign
:num_native_gate_lists
(due to the Method for Clifford RB to return gates-per-Clifford #315 fix above)BinaryRBDesign
:measurements
,signs