Add DAGDependency

[rmoyard]

Summary

I added another form of DAG, the canonical form (reference: https://arxiv.org/abs/1909.05270). The DAGcanonical class can be found at qiskit.dagcircuit.dagcanonical. It is the basis for a new type of DAG. The nodes correspond to operations (gate, measurements, etc...). The edges correspond to non-commutation between two operations. The structure of this class shares similarities with the class DAGCircuit.

Two converters were added qiskit.converters.circuit_to_dagcanonical and qiskit.converters.dagcanonical_to_circuit. The first one creates a DAGcanonical() object from a QuantumCircuit(). The second one does the opposite.

The qiskit.visualization.dag_visualization was adapted (add of a 'type' argument) to include the new form of DAG.

A test for converters was also added at test.python.converters.test_circuit_to_dag_canonical

Simplest example

Bell circuit without measurement;here the edge represents non commutation between the Hadamard gate and Cnot.

from qiskit import QuantumCircuit
from qiskit.converters.circuit_to_dagcanonical import circuit_to_dagcanonical 

def build_bell_no_measurement():
    circuit=QuantumCircuit(2)
    circuit.h(0)
    circuit.cx(0, 1)
    return circuit

circuit =  build_bell_no_measurement()
dag_canonical = circuit_to_dagcanonical(circuit)

dag_canonical.draw(filename='bell.png')

Comments

The function 'commute' in qiskit.dagcircuit.dagcanonical that checks commutation relation can be improved. The lists of commutation and non-commutation are not complete.

Closes #1914 .

[ajavadia]

Thanks @rmoyard, this looks nice and clean at first glance. I'll try to use it a bit to see how it works.

I'm wondering about the plans to use this DAGCanonical in the transpiler pipeline. Do you have any thoughts on that? Right now things are pretty straightforward, DAG goes in a pass, DAG comes out. Do you plan to make some internal conversions to/from? In which case wouldn't it make more sense to have convertors between DAGCircuit <-> DAGCanonicalCircuit?

By the way this PR will close #1914.

[ajavadia]

this seems to be the same function as the one in passes/commutation_analysis.py, perhaps improved?

Actually the CommutationAnalysis pass should not be needed if we can somehow make use of this DAGCanonical rather than DAG.

[rmoyard]

@ajavadia Thank you for the comments and happy new year!

I read more about the passes and transpilers and have now a better understanding of them. Then you are right that it would be better to use converters DAGCircuit <-> DAGCanonicalCircuit. First the main goal that I have (at the moment) for the canonical form is to implement a template matching pass: arXiv:1909.05270 In the current version, they are only converters from circuit to dag canonical and vice versa. Then I will update the code to introduce a converter from the DAGcircuit.

As you said it can be also useful to use this form for commutation related passes. The function for checking commutation is indeed an updated version of the one in CommutationAnalysis. I tried to introduce considerations on which qubits the operation are acting on. I also added a list (which is not complete) for known commutation relations. I also changed the commutation rules for measurement (qubit dependent). This function is still not optimal and any comments are welcome.

[ajavadia]

Ok sounds good. Please add the DAGCircuit <-> DAGCanonical conversions, and @kdk and I will review.

[boschmitt]

You should choose a new name for this data structure because calling it canonical is misleading. We say a unitary matrix representation is canonical because it uniquely describes a computation. In this sense, this form is not canonical. It does have a higher degree of canonicity when compared to the other DAG structure, but it still have different circuits which implement the same computation using a different combination of gates.

[ajavadia]

You should choose a new name for this data structure because calling it canonical is misleading. We say a unitary matrix representation is canonical because it uniquely describes a computation. In this sense, this form is not canonical. It does have a higher degree of canonicity when compared to the other DAG structure, but it still have different circuits which implement the same computation using a different combination of gates.

Yeah I was thinking about the name too. In the paper on which this PR is based, it is called canonical because I guess a given set of gates on a given set of qubits would yield one and only one representation in this form. However I think one could also call it DependencyGraph or DependencyDAG, because it captures execution dependencies between gates that is not captured in the original DAGCircuit. For example see this recent paper (fig 1). There are other examples from literature. Thoughts?

[boschmitt]

Yeah I was thinking about the name too. In the paper on which this PR is based, it is called canonical because I guess a given set of gates on a given set of qubits would yield one and only one representation in this form.

No, it is more constrained than that. It requires the two circuits to be equal up to the set of commutation rules that this structure can capture.

Thoughts?

DependencyDAG is a better name.

Another concern to keep in mind is that maintaining two DAG structures is error prone and a bit annoying. My experience is that lazily annotating dependency information on the DAGCircuit is a cleaner solution. This way there is no need to worry if you are using the right DAG structure for a particular pass.

I don't like the idea of having two quantum circuit representations where calling a method such as successor(node) returns semantically different things. In DAGCircuit, I get a node that will be execute right after; while on this new DAG, I get a node which might be executed right after, but will depend on the arbitrary order I decide for the nodes that commute with node.

[rmoyard]

Thank you for the commentaries!

I agree that the name could be misleading. I could rename it DependencyDAG or maybe CommutationDAG. On the other hand, I'm not sure it's a good idea to build this form in the already existing CircuitDAG. This representation is also useful on its own and not only for compilers related works. The numbers of nodes and edges are not the same between the two DAG. CircuitDAG would become unreadable as we have to duplicate a lot a methods and functions. Furthermore it will be probably more complicated to implement an algorithm (if one would like to only use the 'canonical form'). Personally I think it's better to keep them separated.

@ajavadia I will update the PR with the new converters and updated names by the end of the week.

[itoko]

Hi, @rmoyard . I'm very interested in your DependencyDAG (maybe CommutationDAG).
I just want to confirm its major use case i.e. how users will use it.
No misunderstanding in the followings?

• Main users are transpiler pass developers.
• If they want to use DependencyDAG instead of DAGCircuit within their pass, they convert a DAGCircuit into a DependencyDAG, do something with the DependencyDAG, and create a new DAGCircuit to be returned.
• If they want to implement a DependencyDAG->DependencyDAG transformation algorithm, they need to do DependencyDAG->DAGCircuit after the algorithm (as well as DAGDAGCircuit->DependencyDAG conversion before the algorithm).
• Since we have no means to tell DependencyDAG to another pass (DependencyDAG is not a DAGCircuit), DAGCircuit->DependencyDAG conversion has to be always done in every pass using DependencyDAG.

As for naming, I think transitive reduction is mathematically the right name. I'm not sure TransitiveReduction is suitable as a class name, though.

[rmoyard]

I added the two converters DAGcircuit <-> DAGcanonical. For the name of this new DAG, I will wait for the review of the code and then change every names.

@itoko Thank for your message, you are right about the main uses of the dependency DAG. For uses in transpilers, one will need to first to convert from DAGcircuit() to DependencyDAG() and then convert it back.

[ajavadia]

Ok I made some small tweaks, mostly to the docs, but I think this is ready to go.

There is one case that I'm not sure about: should we code in a rule that Z rotations (e.g. S, Sdg, T, Tdg, Z, RZ) commute with measurements in the Z basis? Right now they are considered non-commuting. I'm not sure if this even affects the template-matching algorithm at all, so I think it can be left to be considered later.

Thanks for the contribution!

[ajavadia]

Actually I just found an issue that I think needs to be addressed.

There are 3 public methods right now that modify the DAG: add_op_node, add_edge, add_successors. If I understand correctly using add_op_node on its own may result in an invalid DAG because the edges are temporarily incorrect. So it always has to be followed by add_edge and add_successors.

I think 2 things need to happen:

1- Change the method names add_edge/add_successors -> update_edges/update_successors and reflect their job, and clearly document their purpose in the docstring.

2-
(a) The only real public modifier method should be add_op_node and it internally calls _update_edges and _update_successors (now private) to always keep the DAG valid.

OR It might be that you are doing this for performance (add a bunch of nodes first, then make them correct all at once)? If this is the case then

(b) I think we should provide an option for add_op_node(lazy=False) so the user explicitly chooses that the update is lazy (default not lazy and it calls those update_ methods). And this should be clearly documented in the method as well.

[ajavadia]

This is not the right argument type, as we are not adding a DAGNode here, but a dictionary of

{'operation': <DAGNode>, 'successors': [], 'predecessors': []}

maybe we should think about a separate class for the nodes of DAGDependency which are distinct from the usual DAGNode used for the DAGCircuit?

The current DAGNode is class consisting of the following attributes:

type, op, name, qargs, cargs, condition

The new node could have the following attributes:

op, name, qargs, cargs, condition, successors, predecessors

(no need for type as we always have 'op' types)

What do you think?

[rmoyard]

Concerning commutation in the Z-basis, I prefer to let this as non-commuting at least for the moment in order not to affect templating matching.

Yes it could be great to have a separate node type for the DAGDependency(), then all attributes are at the same level. I will do this change and also the one for the _update_edge() as soon as possible.

Thank you for your comments.