-
Notifications
You must be signed in to change notification settings - Fork 160
Add optional prune_classical_qubits setting for circuit generation
#2802
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
base: main
Are you sure you want to change the base?
Conversation
|
I'm a bit uneasy about this feature as it is. The trimmed circuit is not equivalent to the original and it may be confusing. For example, I can create this circuit: operation Foo() : Result {
use qs = Qubit[2];
X(qs[0]);
CNOT(qs[0], qs[1]);
H(qs[1]);
Ry(Std.Math.PI() / 2.0, qs[1]);
X(qs[0]);
MResetZ(qs[1])
}In this circuit the first qubit is trimmable, but the second is not. We can compare original and resulting circuits. |
trim setting for circuit generationprune_classical_qubits setting for circuit generation
920cd6c to
260bf32
Compare
@DmitryVasilevsky, I believe the new logic handles this appopriately. Using your same sample code, the output now looks the way you were hoping, keeping the circuit equivalent: I'll need more tests to ensure this across different inputs though! |
This change adds a parameter to Q# and OpenQASM circuit generation called `trim` that will generate a simplified circuit diagram where qubits that are unused or purely classical (never enter superposition) are removed. This can be handy for places where the program comes from generated logic that may use more qubits than strictly needed. This is purely for visualizing the simplified circuit and does not perform any kind of execution optimization.
…, still need tests
260bf32 to
88b2c87
Compare
|
@DmitryVasilevsky @minestarks This is now ready for review with the updated behavior based on your feedback. No rush to get this in soon though. |
| finish_circuit(qubits, operations, num_qubits, source_lookup) | ||
| } | ||
|
|
||
| fn should_keep_operation_mut(&self, op: &mut Operation) -> bool { |
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 this operate on OperationOrGroup before we transform it into Operation? OperationOrGroup is the "working" data structure which is more ergonomic to manipulate, and already has methods like all_qubits() . Operation is essentially the final "wire format" that we serialize to JSON, so we have to keep it stable but is kind of annoying to manipulate. You shouldn't have to change this function too much.
| #[must_use] | ||
| pub fn targets_mut(&mut self) -> &mut Vec<Register> { | ||
| match self { | ||
| Operation::Measurement(m) => &mut m.qubits, | ||
| Operation::Unitary(u) => &mut u.targets, | ||
| Operation::Ket(k) => &mut k.targets, | ||
| } | ||
| } | ||
|
|
||
| #[must_use] | ||
| pub fn all_qubits(&self) -> Vec<Register> { | ||
| match self { | ||
| Operation::Measurement(m) => m.qubits.clone(), | ||
| Operation::Unitary(u) => { | ||
| let mut qubits = u.targets.clone(); | ||
| qubits.extend_from_slice(&u.controls); | ||
| qubits | ||
| } | ||
| Operation::Ket(k) => k.targets.clone(), | ||
| } | ||
| } | ||
|
|
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.
Please see above comment - if you use OperationOrGroup when manipulating the circuit, you shouldn't have to add these methods
| circuit_builder: OperationListBuilder, | ||
| next_result_id: usize, | ||
| user_package_ids: Vec<PackageId>, | ||
| superposition_qubits: FxHashSet<usize>, |
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.
These are "wire ID"s right? Can we leave the type as QubitWire for clarity?
| } | ||
|
|
||
| fn mark_qubit_in_superposition(&mut self, wire: QubitWire) { | ||
| self.superposition_qubits.insert(wire.into()); |
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.
Perhaps we can have an assert here (and in flip_classical_qubit below) that the prune_classical_qubits config is set to true. Reading this, I get a bit nervous that we might be doing this work regardless of config flag. I can see you obviously took it into consideration this in this PR, but it feels like it could be accidentally regressed with a future change.
| circuit_builder: OperationListBuilder, | ||
| next_result_id: usize, | ||
| user_package_ids: Vec<PackageId>, | ||
| superposition_qubits: FxHashSet<usize>, |
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.
Since this is expected to be very dense with a range of 0..max_qubits ... is FxHashSet<usize> still the right choice or should we use Vec<bool>?
(I can totally buy "it doesn't matter for circuits this small" though)
| generation_method: Option<CircuitGenerationMethod>, | ||
| source_locations: Option<bool>, | ||
| group_by_scope: Option<bool>, | ||
| prune_classical_qubits: bool, |
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.
I can't process why this flag looks different than the others. Why not Option<bool>? Should the others have been bool?
| source_locations: config.source_locations, | ||
| max_operations: config.max_operations, | ||
| group_by_scope: config.group_by_scope, | ||
| prune_classical_qubits: false, |
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.
Don't we want to plumb this up to VS Code?
| // We need to pass the original number of qubits, before any trimming, to finish the circuit below. | ||
| let num_qubits = qubits.len(); |
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.
Just curious, why do we need the original num_qubits? Should we need it? I'm not familiar with the code in operation_list_to_grid so I'm genuinely asking to see if there's something that can be improved there.
minestarks
left a comment
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 good, but lots of little suggestions
This is better than the original approach after including @DmitryVasilevsky's feedback, but I still find this confusing. I do like the underlying motivation for the feature though. Could one alternative highlight qubits that are 100% "classical" (either in |0> or |1> state) with some color. Their value might affect different gates/operations in different ways, e.g., |0> on a CNOT's control removes the gate, |1> on a CNOT's control removes the control (other gates behave differently). Further, not meant to be part of this PR, but if we have logic to determine these values, we could also apply the optimization to the circuit and then visualize it (and have the same benefits for other tasks, to, e.g., speed up simulation, reduce resources, ...) |




This change adds a parameter to Q# and OpenQASM circuit generation called
prune_classical_qubitsthat will generate a simplified circuit diagram where qubits that are unused or purely classical (never enter superposition) are removed. This can be handy for places where the program comes from generated logic that may use more qubits than strictly needed. This is purely for visualizing the simplified circuit and does not perform any kind of execution optimization.So a program like this that has nine qubits but doesn't really use all of them:

will by default show the full circuit as it does today:

but with

prune_classical_qubits = Truewill remove the purely classical q1, q2, q4, q5, and q8: