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
add DropNegligible transpiler pass #12384
base: main
Are you sure you want to change the base?
Conversation
Pull Request Test Coverage Report for Build 9038654167Warning: 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
💛 - Coveralls |
28521ea
to
e442e7a
Compare
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 think having a pass like this in the library makes a lot of sense. We don't want to run it by default, but if you know it makes sense in your use case then having this available is useful. We could even look at adding it to the end of the init stage in the preset pass managers and tying the atol
value to the approximation_degree
value somehow.
if not isinstance(node.op, DROP_NEGLIGIBLE_GATE_CLASSES): | ||
continue |
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 you're using a fixed set of gates in this PR one thing you should do here is before looping is check if the names of the gate classes intersect with dag.count_ops()
. That'll be much faster in the negative case.
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.
Doesn't dag.count_ops
also need to iterate through the nodes of the dag?
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.
DAGCircuit
maintains a dictionary of counts that it updates whenever an op is added or removed from the circuit. This is explicitly to support cases like this to do constant time checks for the instructions presence in the circuit. The only exceptions are if you set recurse=True
(which is the default) and there are any control flow ops in the circuit.
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.
dag.count_ops()
stores strings, but I'm using the gate class itself to check the gate type. Any advice? Should I also just use strings? That doesn't seem robust because any gate can have any arbitrary name. But I guess there's many places in Qiskit which assume to know the type of a gate based on its name.
if np.allclose(node.op.params, 0, atol=self.atol): | ||
dag.remove_op_node(node) |
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'm wondering if we want to generalize this for any Gate
object, we could do something similar to this but instead compare node.op.to_matrix()
against an identity matrix with the tolerance.
That being said doing this will obviously be slower though. If we're concerned about runtime performance I'd probably suggest moving this away from using np.allclose()
and just do something like:
if np.allclose(node.op.params, 0, atol=self.atol): | |
dag.remove_op_node(node) | |
if all(math.isclose(x, 0, atol=self.atol) for x in node.op.params): | |
dag.remove_op_node(node) |
because I expect the numpy overhead of converting params to an array and all the checking involved with that will be slower that just checking each value in params manually, especially as in general there won't be very many parameters for a gate.
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 idea to use the built-ins instead of Numpy, done.
I don't think we should try to construct a gate's matrix, due to the performance overhead, and also because it's not feasible for all gates (cost is exponential in the number of qubits). However, I do think we should let the user pass additional gate classes to check.
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've added an argument for passing additional gate types.
I like the general idea to drop negligible gates but think this implementation is not optimal. |
The concern I have with just relying on the |
|
3718d9a
to
2205d32
Compare
2205d32
to
9f57938
Compare
try:
decomp = TwoQubitWeylDecomposition(node.op, fidelity=fid, _specialization=_specializations.IdEquiv)
except QiskitError:
pass
else:
<replace dag node with 1q gates from decomp> |
Doing linear algebra is too expensive when the aim is just to drop gates with small angles. |
Summary
Fixes #8654.
Draft PR because I would like feedback before adding more code.
Details and comments
The pass removes a gate if:
DROP_NEGLIGIBLE_GATE_CLASSES
.1e-8
).DROP_NEGLIGIBLE_GATE_CLASSES
should list gates with the property that if all of their parameters are close to zero, then the gate's effect is small. It does not guarantee any error bounds based on the tolerance, so in principle this pass can drop gates whose effect is not actually negligible. In practice, I don't expect it to be an issue.So far, I've only added a few gates to
DROP_NEGLIGIBLE_GATE_CLASSES
. If people agree with this approach, I'll go through the rest of the gate library and add the appropriate ones.@mtreinish @ajavadia thoughts?