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
Update and expand DAGDependency drawer #11103
Conversation
One or more of the the following people are requested to review this:
|
Pull Request Test Coverage Report for Build 6635363212
💛 - Coveralls |
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.
In general, this looks a completely clear win in usability to me. I haven't used DAGDependency
much though, so it'd be good to get a second opinion. @alexanderivrii, does the new output format look good to you here?
Can we have a "feature" release note for this improvement to the drawers as well?
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.
This one isn't a DAGDependency
, right?
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.
Right. I was going to use a 1% diff on the image tests, but this one was at 6%, so I thought I'd update the reference image. Turns out after doing this it was still at 6% and the DAGDependency image was at 8% on the CI run. So I don't know what the difference is between the fonts, image libraries, etc. between my local and CI. I have the recommended version of graphviz. So we could just leave the old one.
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 mean, the new one looks more like it's intended and it passes CI, so I'm fine leaving it.
I'll add a reno. |
Reno added in 53381b3. |
Thanks @enavarro51, I really like the new output. The code changes look good as well. |
* Update and expand dag dependency drawer * Increase tolerance * Add conditions and truncate long lists * Lint * Cyclic import * Lint * Reno
Summary
Fixes #9066
Details and comments
This PR updates the
DAGDependency
section of theqiskit/visualization/dag_drawer.py
.node.qargs
andnode.cargs
have been added to each node display to indicate which qubits and clbits the instruction acts on.DAGCircuit
drawings.This circuit
produces this circuit drawing
and this graph drawing