Skip to content
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

Replace DAGNode class with OpNode, InNode, and OutNode classes #6567

Merged
merged 55 commits into from
Aug 3, 2021

Conversation

enavarro51
Copy link
Contributor

@enavarro51 enavarro51 commented Jun 13, 2021

Summary

Fixes #6493

Details and comments

This PR replaces the qiskit.dagcircuit.DAGNode class with 3 classes - OpNode, InNode, and OutNode - and turns DAGNode into the parent of the 3 classes. The type, op, and wire kwargs to DAGNode have been deprecated.

Copy link
Member

@kdk kdk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @enavarro51 , this looks good so far. One question on the DAGNode parent class.

qiskit/dagcircuit/dagnode.py Outdated Show resolved Hide resolved
qiskit/dagcircuit/dagnode.py Outdated Show resolved Hide resolved
@kdk kdk added this to the 0.18 milestone Jun 15, 2021
@kdk kdk modified the milestones: 0.18, 0.19 Jun 15, 2021
Copy link
Member

@kdk kdk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @enavarro51 , this looks good so far. Few comments below.

qiskit/dagcircuit/dagnode.py Outdated Show resolved Hide resolved
qiskit/dagcircuit/__init__.py Outdated Show resolved Hide resolved
qiskit/dagcircuit/dagnode.py Outdated Show resolved Hide resolved
qiskit/transpiler/passes/layout/apply_layout.py Outdated Show resolved Hide resolved
Copy link
Member

@kdk kdk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thanks for the updates @enavarro51 . One more question about deprecation warnings, then I think this on hold for #6694 .

qiskit/dagcircuit/dagnode.py Show resolved Hide resolved
Copy link
Member

@kdk kdk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for all the updates @enavarro51 , and for working out all the edge cases. Few small comments, then this looks good to go.

qiskit/dagcircuit/dagcircuit.py Show resolved Hide resolved
qiskit/dagcircuit/dagcircuit.py Outdated Show resolved Hide resolved
qiskit/dagcircuit/dagcircuit.py Outdated Show resolved Hide resolved
qiskit/dagcircuit/dagcircuit.py Outdated Show resolved Hide resolved
qiskit/dagcircuit/dagcircuit.py Outdated Show resolved Hide resolved
Copy link
Member

@kdk kdk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the updates @enavarro51 !

@kdk kdk added Changelog: New Feature Include in the "Added" section of the changelog Changelog: Deprecation Include in "Deprecated" section of changelog labels Aug 3, 2021
@kdk kdk merged commit 8e7ba42 into Qiskit:main Aug 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: Deprecation Include in "Deprecated" section of changelog Changelog: New Feature Include in the "Added" section of the changelog performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Performance regression caused by dagnode op property
2 participants