New transpiler techniques (gate optmization): U3GateFusion, RotationGateFusion and InverseCancellation #1606
Conversation
for more information, see https://pre-commit.ci
|
@alecandido is there a way for external PRs to trigger the CI and run the tests? |
| if not gate.qubits: | ||
| continue |
There was a problem hiding this comment.
Why is this necessary?
| def update_previous_gates(qubits, idx, gate): | ||
| """Helper function to update previous gate tracking.""" | ||
| for q in qubits: | ||
| previous_gates[q] = (idx, gate) | ||
|
|
||
| def clear_previous_gates(qubits): | ||
| """Helper to clear tracking for given qubits.""" | ||
| for q in qubits: | ||
| previous_gates[q] = None |
There was a problem hiding this comment.
I advise to not define functions inside other functions unless it is strictly necessary
| if not gate.init_args: | ||
| continue |
There was a problem hiding this comment.
Again, is this necessary? Why would a gate in the queue of a circuit not have init_args defined?
| def __find_pos_rm(self, newCircuit: Circuit): | ||
| """ | ||
| Identifies and marks pairs of inverse gates that can be removed from the circuit. | ||
|
|
||
| Args: | ||
| newCircuit: The Circuit. | ||
|
|
||
| """ |
There was a problem hiding this comment.
The python convention is that names like newCircuit are reserved for classes, while simple variables should be all lowercase and separated by _, e.g. new_circuit.
Moreover, docstrings should be more descriptive and also follow the same typing patterns as the rest of qibo. Example below.
| def __find_pos_rm(self, newCircuit: Circuit): | |
| """ | |
| Identifies and marks pairs of inverse gates that can be removed from the circuit. | |
| Args: | |
| newCircuit: The Circuit. | |
| """ | |
| def __find_pos_rm(self, new_circuit: Circuit): | |
| """Identifies and marks pairs of inverse gates that can be removed from the circuit. | |
| Args: | |
| new_circuit (:class:`qibo.models.circuit.Circuit`): Circuit to have gates identified. | |
| """ |
| ) | ||
| self.pos_rm_inv_gate = [] | ||
|
|
||
| def __call__(self, circuit: Circuit) -> Circuit: |
| secondary_match = ( | ||
| all( | ||
| previous_gates[q] is not None | ||
| and self.__sameGates(gate, previous_gates[q][1]) | ||
| for q in other_qubits | ||
| ) | ||
| if other_qubits | ||
| else True | ||
| ) |
There was a problem hiding this comment.
Can you break this into multiple lines to improve readability? Thanks
| if previous_gates[primary_qubit] is None: | ||
| update_previous_gates(gate.init_args, i, gate) | ||
| else: | ||
| if isinstance(gate, self.inverse_gates): |
There was a problem hiding this comment.
| if previous_gates[primary_qubit] is None: | |
| update_previous_gates(gate.init_args, i, gate) | |
| else: | |
| if isinstance(gate, self.inverse_gates): | |
| if previous_gates[primary_qubit] is None: | |
| update_previous_gates(gate.init_args, i, gate) | |
| elif isinstance(gate, self.inverse_gates): |
| else: | ||
| update_previous_gates(gate.init_args, i, gate) |
There was a problem hiding this comment.
| else: | |
| update_previous_gates(gate.init_args, i, gate) | |
| else: | |
| update_previous_gates(gate.init_args, i, gate) |
| return (gate1.name, gate1.init_args, gate1.init_kwargs) == ( | ||
| gate2.name, | ||
| gate2.init_args, | ||
| gate2.init_kwargs, | ||
| ) |
There was a problem hiding this comment.
| return (gate1.name, gate1.init_args, gate1.init_kwargs) == ( | |
| gate2.name, | |
| gate2.init_args, | |
| gate2.init_kwargs, | |
| ) | |
| tuple_1 = (gate1.name, gate1.init_args, gate1.init_kwargs) | |
| tuple_2 = (gate2.name, gate2.init_args, gate2.init_kwargs) | |
| return bool(tuple_1 == tuple_2) |
| self.rotated_gates = gates.RX | gates.RY | gates.RZ | ||
| self.gates = [] | ||
|
|
||
| def __call__(self, circuit: Circuit) -> Circuit: |
| if isinstance(prev_gate, type(gate)): | ||
| tmp_gate = type(gate)( |
There was a problem hiding this comment.
| if isinstance(prev_gate, type(gate)): | |
| tmp_gate = type(gate)( | |
| if isinstance(prev_gate, gate.__class__): | |
| tmp_gate = gate.__class__( |
| self.gates.extend(g for g in previous_gates if isinstance(g, gates.U3)) | ||
|
|
||
| @staticmethod | ||
| def __extract_u3_params(U): |
There was a problem hiding this comment.
It is recommended that variable names should be at least 3 letters. Ideally, more descriptive.
| class InverseCancellation(Optimizer): | ||
| """ | ||
| Identifies and removes pairs of adjacent gates from a quantum circuit. | ||
| """ | ||
|
|
||
| def __init__(self): | ||
| self.inverse_gates = ( | ||
| gates.H | gates.Y | gates.Z | gates.X | gates.CNOT | gates.CZ | gates.SWAP | ||
| ) | ||
| self.pos_rm_inv_gate = [] |
There was a problem hiding this comment.
Move the class docstrings to the __init__ method and add any Args when necessary. This and the other classes.
alecandido
added
run-workflow
Yes, there is: it is similar to what you imagined. It is not the |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1606 +/- ##
==========================================
- Coverage 99.58% 98.97% -0.61%
==========================================
Files 76 76
Lines 11322 11447 +125
==========================================
+ Hits 11275 11330 +55
- Misses 47 117 +70
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Thanks so much for your reviews, @renatomello. I am working on the code to add them |
We have worked on issue #1588 and added new techniques in transpiler optimization:
Checklist: