Skip to content

Adds method to list nodes on a wire#1975

Merged
ajavadia merged 8 commits into
Qiskit:masterfrom
maddy-tod:gates-on-wire
Mar 25, 2019
Merged

Adds method to list nodes on a wire#1975
ajavadia merged 8 commits into
Qiskit:masterfrom
maddy-tod:gates-on-wire

Conversation

@maddy-tod
Copy link
Copy Markdown
Contributor

@maddy-tod maddy-tod commented Mar 18, 2019

Summary

Fixes #1502

Adds a method nodes_on_wire() to the DAGCircuit. This method yields all the nodes in the DAG which affect the given wire. The method takes 2 parameters -
wire : the wire to be looked at
only_ops(optional) : default to False, if True then only the op nodes are returned, otherwise all nodes on the wire are returned.

This will be affected by #1815 and will be updated to return DAGNodes instead.

@diego-plan9
Copy link
Copy Markdown
Member

@maddy-tod while you are working on the file, can you rename qiskit/dagcircuit/_dagcircuit.py to qiskit/dagcircuit/dagcircuit.py (ie. removing the underscore) at a convenient time (for example, as the last commit in this PR)? Since you are one of most active contributors working on the file, it would make it smoother if you took care of it during one of your PRs, to avoid some conflicts.

Comment thread qiskit/dagcircuit/_dagcircuit.py Outdated
Comment thread qiskit/dagcircuit/_dagcircuit.py Outdated
@maddy-tod
Copy link
Copy Markdown
Contributor Author

I have also renamed qiskit/dagcircuit/_dagnode.py to qiskit/dagcircuit/dagnode.py after #1815 .

@ajavadia ajavadia force-pushed the gates-on-wire branch 2 times, most recently from 497cca6 to c123950 Compare March 23, 2019 10:10
@ajavadia
Copy link
Copy Markdown
Member

ajavadia commented Mar 25, 2019

This looks good, there were some conflicts that I resolved.

However during the review, I noticed that the DAGNode.__eq__ method is defined so that it can compare DAGNode against int. This was probably done due to some convenience, but I think it is a bit of an unexpected behavior to compare objects of different types. It took me a while to understand why the test was comparing against ints, when the nodes_on_wire() method is returning DAGNodes. Can we fix this in a follow up PR? Possibly related to #2006 (@1ucian0).

@ajavadia ajavadia merged commit ece61ef into Qiskit:master Mar 25, 2019
@maddy-tod
Copy link
Copy Markdown
Contributor Author

@ajavadia That was done to allow for easier testing (there was a comment in the code about it at some stage but it must have been removed). I think this was mainly because the older version of DAGNode.__eq__ only allowed for identical DAGNode objects to compare to equal. This meant that we couldn't test that, for example, a method returned the nodes in the right order without looking at the node_id, as if we simply made identical objects to what we were expecting they wouldn't compare as equal. It was neatest to put it in DAGNode.__eq__ as otherwise all the testing would have to access the private variable _node_id.

The work @1ucian0 is doing should fix this but we could also refactor it out by having a method to return the _node_id/compare a given val with the _node_id to see if it is equal.

lia-approves pushed a commit to edasgupta/qiskit-terra that referenced this pull request Jul 30, 2019
* implemented a function to list the operations on a given wire

* CHANGELOG

* Updated function to make clear that it returns nodes not ops, and then added parameter to dictate if only op nodes are required

* Updated function to make clear that it returns nodes not ops, and then added parameter to dictate if only op nodes are required

* Linting!

* Linting

* Renamed _dagcircuit to dagcircuit

* Refactoring
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Need a data structure to list the gates on qubits

4 participants