DAGCircuit layers uses a shallow copy #378

eddieschoute · 2018-03-30T21:44:54Z

In my profiling the DAGCircuit.layers() function came forward as the main culprit to my runtime, at about 90% of the total run time. 60% of the total time is spent in a the function _add_wire which is of course a bit silly considering the trivial thing it does. Turns out that DAGCircuit constructs a full copy of itself besides the gates, which turns out to be very slow. Instead of making a full copy it now returns a shallow copy.

Description

The layers function now constructs a shallow copy of DAGCircuit and modifies it such that it only contains the nodes of the current layer. That being: All input and output nodes disregarding whether they are used or not, and exactly one layer of op_nodes.

Profiling results

On a relatively small circuit containing a 1000 gates my runtime goes from 22 minutes on the dag-generators branch (#371 ) to 16 minutes by using this change. The total runtime of layers goes from 18 minutes to 9 minutes. Still not quite satisfactory but closer.

Motivation and Context

I use the layers functionality extremely often to iterate through the layers and compare the cost of running a circuit between slight variations of the circuit.

How Has This Been Tested?

I added a test case for the layers functionality. (I was surprised to see that nothing was being tested before.) The test case is the only one that fails. There is some strange behavior in how the multi_graph in the DAGCircuit is constructed.

I'm inspecting the DAG multigraph when making a test and it just doesnt make sense. I have nodes that have edges to nodes that should be unrelated. E.g

dag.apply_operation_back('h', [qubit0], [], [], condition)
dag.apply_operation_back('cx', [qubit0, qubit1], [],
                        [], condition)
dag.apply_operation_back('measure', [qubit1], [clbit1], [], condition)
dag.apply_operation_back('x', [qubit1], [], [], clbit1)
dag.apply_operation_back('measure', [qubit0], [clbit0], [], condition)
dag.apply_operation_back('measure', [qubit1], [clbit1], [], condition)

from test_dagcircuit.py will have some strange edges. It has an edge from clbit0 to the x operation being performed on qubit1 with clbit1. It seems related to how _def_bits_in_condition adds all classical bits of a register to the operation dependencies.

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Breaking: layers now is a generator. Supersedes #371

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
All new and existing tests passed.

eddieschoute · 2018-03-30T22:10:28Z

I would also like to not that my total runtime for the same problem goes down to 4 minutes if I use the new multigraph_layers function that does not construct intermediate DAGCircuits.

diego-plan9 · 2018-06-05T12:38:14Z

@eddieschoute, can you check if the latest PR merged #523 by @ajavadia requires some changes to this PR, and update about the WIP status? It seems it has fallen off the radar a bit and might be a bit not fully compatible with the latest changes, but it would be great to be able to solve the issue and provide a performance bump thanks to your contribution.

eddieschoute · 2018-06-05T13:37:26Z

@diego-plan9 I can look at it again, but there is another bug that will make tests fail (#382) that needs to be addressed too.

diego-plan9 · 2018-06-05T13:40:22Z

Thanks @eddieschoute ! Perhaps deviating a bit to tackle the bug in another PR and then coming back to this one would be more convenient?

eddieschoute · 2018-06-05T13:57:29Z

Of course. But I figured you wouldn't want to merge changes into the master branch that have failing tests.

(I am not planning on addressing the issue myself.)

eddieschoute · 2018-06-05T14:00:32Z

I have resolved the conflicts, but a close look is needed if it supports the same functionality that has been added to the layers function. I saw that now partition now has new filters so I added

                if op_node[1]["name"] not in {"barrier", "snapshot", "save", "load", "noise"}

None of the tests (besides that bug) seem to have broken, except for test_qobj_measure_opt (python.test_qasm_simulator_cpp.TestLocalQasmSimulatorCpp), which does not seem related.

diego-plan9 · 2018-06-05T14:15:56Z

Great, thanks!

Of course. But I figured you wouldn't want to merge changes into the master branch that have failing tests.

Indeed 👍

(I am not planning on addressing the issue myself.)

No worries, it was unclear to me if you were meant to handle the bugfix yourself - I'm labeling his PR as on hold until we have a fix in place.

Can you confirm me that this PR would then be ready for review and out of WIP state?

eddieschoute · 2018-06-05T14:40:28Z

Yes it is ready. I have removed the WIP status.

delapuente · 2018-08-08T08:39:57Z

@eddieschoute, would you mind to open an issue before creating the PR the next time, please? This way we ease the tracking.

* The layers function used to return a normally constructed DAGCircuit object for each layer. * Now instead it constructs a shallow copy that provides a limited "view" onto the operations being performed in a layer. * Added multigraph_layers method to yield the nodes in the multigraph layer-by-layer. This functionality is being duplicated inside DAGCircuit and can be refactored out.

* Added a return statement if a layer was going to be empty, e.g. when containing only output nodes. * Cleanup of code.

* The next_layer was being set to empty before being stored for the next iteration.

* Multigraph_layers was not taking multiplicity of edges into account and thus not emitting certain edges at the right times. * Added test case for layers

ajavadia · 2018-10-06T14:47:30Z

@eddieschoute I rebased this branch and fixed the test according to the explanation I gave in #382 (comment).

It looks good. Thanks for the contribution.

* DAGCircuit layers returns shallow copies * The layers function used to return a normally constructed DAGCircuit object for each layer. * Now instead it constructs a shallow copy that provides a limited "view" onto the operations being performed in a layer. * Added multigraph_layers method to yield the nodes in the multigraph layer-by-layer. This functionality is being duplicated inside DAGCircuit and can be refactored out. * Layers would not properly emit a StopIteration * Added a return statement if a layer was going to be empty, e.g. when containing only output nodes. * Cleanup of code. * Fixed bug in multigraph_layers * The next_layer was being set to empty before being stored for the next iteration. * Take multiedges into account * Multigraph_layers was not taking multiplicity of edges into account and thus not emitting certain edges at the right times. * Added test case for layers * Fix naming of yielded dict. And call-sites of layers * Fix type of partition field in layers * Adhering to style guidelines * Changed maps/filters to comprehensions * fix test according to explanation in #382

* DAGCircuit layers returns shallow copies * The layers function used to return a normally constructed DAGCircuit object for each layer. * Now instead it constructs a shallow copy that provides a limited "view" onto the operations being performed in a layer. * Added multigraph_layers method to yield the nodes in the multigraph layer-by-layer. This functionality is being duplicated inside DAGCircuit and can be refactored out. * Layers would not properly emit a StopIteration * Added a return statement if a layer was going to be empty, e.g. when containing only output nodes. * Cleanup of code. * Fixed bug in multigraph_layers * The next_layer was being set to empty before being stored for the next iteration. * Take multiedges into account * Multigraph_layers was not taking multiplicity of edges into account and thus not emitting certain edges at the right times. * Added test case for layers * Fix naming of yielded dict. And call-sites of layers * Fix type of partition field in layers * Adhering to style guidelines * Changed maps/filters to comprehensions * fix test according to explanation in Qiskit#382

eddieschoute mentioned this pull request Apr 2, 2018

DAGCircuit adds unneeded classical dependencies #382

Closed

eddieschoute requested review from ajavadia, atilag, diego-plan9 and ewinston as code owners June 5, 2018 13:46

diego-plan9 added the on hold Can not fix yet label Jun 5, 2018

eddieschoute changed the title ~~WIP: DAGCircuit layers uses a shallow copy~~ DAGCircuit layers uses a shallow copy Jun 5, 2018

ewinston self-assigned this Jun 5, 2018

delapuente mentioned this pull request Aug 8, 2018

Improve performance of DAGCircuit#layers method #749

Closed

ajavadia self-assigned this Oct 5, 2018

ajavadia removed the on hold Can not fix yet label Oct 5, 2018

eddieschoute requested review from 1ucian0, delapuente, jaygambetta, mtreinish, nonhermitian and taalexander as code owners October 6, 2018 10:36

ajavadia changed the base branch from master to transpiler October 6, 2018 10:43

eddieschoute added 3 commits October 6, 2018 10:34

Layers would not properly emit a StopIteration

b0a2ed1

* Added a return statement if a layer was going to be empty, e.g. when containing only output nodes. * Cleanup of code.

Fixed bug in multigraph_layers

457c238

* The next_layer was being set to empty before being stored for the next iteration.

eddieschoute and others added 6 commits October 6, 2018 10:34

Take multiedges into account

bad9522

* Multigraph_layers was not taking multiplicity of edges into account and thus not emitting certain edges at the right times. * Added test case for layers

Fix naming of yielded dict. And call-sites of layers

b051f60

Fix type of partition field in layers

ded71f3

Adhering to style guidelines

1143163

Changed maps/filters to comprehensions

1e8e865

fix test according to explanation in #382

a021271

ajavadia approved these changes Oct 6, 2018

View reviewed changes

ajavadia merged this pull request into Qiskit:transpiler Oct 8, 2018

1ucian0 added this to dag api in Transpiler Oct 10, 2018

ajavadia moved this from dag api to done in Transpiler Oct 11, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DAGCircuit layers uses a shallow copy #378

DAGCircuit layers uses a shallow copy #378

eddieschoute commented Mar 30, 2018 •

edited by delapuente

eddieschoute commented Mar 30, 2018

diego-plan9 commented Jun 5, 2018

eddieschoute commented Jun 5, 2018

diego-plan9 commented Jun 5, 2018

eddieschoute commented Jun 5, 2018

eddieschoute commented Jun 5, 2018

diego-plan9 commented Jun 5, 2018

eddieschoute commented Jun 5, 2018

delapuente commented Aug 8, 2018

ajavadia commented Oct 6, 2018

DAGCircuit layers uses a shallow copy #378

DAGCircuit layers uses a shallow copy #378

Conversation

eddieschoute commented Mar 30, 2018 • edited by delapuente

Description

Profiling results

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

eddieschoute commented Mar 30, 2018

diego-plan9 commented Jun 5, 2018

eddieschoute commented Jun 5, 2018

diego-plan9 commented Jun 5, 2018

eddieschoute commented Jun 5, 2018

eddieschoute commented Jun 5, 2018

diego-plan9 commented Jun 5, 2018

eddieschoute commented Jun 5, 2018

delapuente commented Aug 8, 2018

ajavadia commented Oct 6, 2018

eddieschoute commented Mar 30, 2018 •

edited by delapuente