Adds support to PassManager drawer to display stages #9128

psschwei · 2022-11-14T20:23:29Z

Signed-off-by: Paul S. Schweigert paul@paulschweigert.com

Summary

Adds support to PassManager drawer to display stages

Details and comments

The ask was to add an outer box around the passes for each stage. This meant implementing a new draw() method for StagedPassManager that:

builds a subgraph for each stage
links those subgraphs together (i.e. connects the last node in one stage to the first node in the next)
returns the final graph

qiskit-bot · 2022-11-14T20:23:34Z

Thank you for opening a new pull request.

Before your PR can be merged it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient.

While you're waiting, please feel free to review other open PRs. While only a subset of people are authorized to approve pull requests for merging, everyone is encouraged to review open pull requests. Doing reviews helps reduce the burden on the core team and helps make the project's code better for everyone.

One or more of the the following people are requested to review this:

@Qiskit/terra-core
@enavarro51

coveralls · 2022-11-15T19:53:14Z

Pull Request Test Coverage Report for Build 4059859471

8 of 63 (12.7%) changed or added relevant lines in 3 files are covered.
20 unchanged lines in 1 file lost coverage.
Overall coverage decreased (-0.02%) to 85.232%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
qiskit/transpiler/passmanager.py	1	3	33.33%
qiskit/visualization/pass_manager_visualization.py	6	59	10.17%

Files with Coverage Reduction	New Missed Lines	%
src/results/marginalization.rs	20	88.76%

Totals
Change from base Build 4056548338:	-0.02%
Covered Lines:	67033
Relevant Lines:	78648

💛 - Coveralls

psschwei · 2022-11-18T16:03:17Z

cc @mtreinish

javabster

Hi @psschwei thanks for your work on this! @mtreinish is away on vacation so it might be a little while before he reviews it. In the meantime could you add some screenshots to this PR of how the drawer renders the result with your changes?

qiskit/visualization/pass_manager_visualization.py

releasenotes/notes/staged-pass-manager-drawer-f1da0308895a30d9.yaml

psschwei · 2022-12-12T19:50:45Z

could you add some screenshots to this PR of how the drawer renders the result with your changes?

Here's a very short example (that I borrowed from somewhere in the Qiskit docs, but didn't bookmark exactly where):

from qiskit.providers.fake_provider import FakeLagosV2
from qiskit.transpiler.preset_passmanagers import generate_preset_pass_manager
backend=FakeLagosV2()
pass_manager = generate_preset_pass_manager(3, backend)
pass_manager.draw()

Here's what it looked like before these changes:

and here's the result with the changes from this PR:

psschwei · 2023-01-11T20:43:55Z

@mtreinish @1ucian0 gentle ping

mtreinish

Sorry for the delay in review, I took some time off at the end of 2022 and this fell through the cracks before I took off.

Overall this LGTM this is basically what I had in mind and it'll be great to have this visualization improved to properly show stages. I had a couple inline comments. But the biggest thing I think is the code duplication. Right now the inner subgraph being built is essentially copying what pass_manager_drawer() is doing. I feel like it would be best to just have a private function that is given a Cluster and PassManager (which each stage is) and then adds the subgraphs, nodes, and edges to it. Then most of the code will be shared between the two functions.

qiskit/visualization/pass_manager_visualization.py

psschwei · 2023-01-18T17:05:40Z

qiskit/visualization/pass_manager_visualization.py

@@ -179,17 +279,3 @@ def pass_manager_drawer(pass_manager, filename=None, style=None, raw=False):
        if filename:
            image.save(filename, "PNG")
        return image
-
-
-def _get_node_color(pss, style):


note: this is still there on line 101 or thereabouts, git diff just makes it look like a deletion / readdition

psschwei · 2023-01-18T17:44:45Z

seems there's an issue where the image won't load for a staged draw... looking into that now

edit: looks to be something with utils._trim(image)... wonder if something changed there...

edit2: problem seems to be with this line, as the image renders properly if I comment out the line: https://github.com/Qiskit/qiskit-terra/blob/b4e10d5bb2750cc42e8787c17544d97d07158587/qiskit/visualization/utils.py#L25

digging a little more, looks like the issue is with the offset... existing value of -100 won't render in a notebook:

whereas updating the offset to -0.75 will render:

that said, I don't know the codebase well enough to be able to say if changing the offset is a good idea. for now, I've just removed the trim call in the drawer output, but am open to something else if you think that makes more sense

psschwei · 2023-01-18T18:40:39Z

qiskit/visualization/pass_manager_visualization.py

@@ -174,22 +274,7 @@ def pass_manager_drawer(pass_manager, filename=None, style=None, raw=False):
        graph.write_png(tmppath)  # pylint: disable=no-member

        image = Image.open(tmppath)
-        image = utils._trim(image)


opted for removing the trim versus messing with the offset (as per #9128 (comment))

This makes sense to me. The trim here isn't super necessary, IIRC (and I probably don't since it's been a while since I dove down these code paths) this was added primarily for the latex circuit visualization which ends up with a pillow image object for an entire pdf page and the image is trimmed to avoid excess whitespace. For these image files generated by graphviz we don't have this concern, so the trim call was mostly superfluous.

Signed-off-by: Paul S. Schweigert <paul@paulschweigert.com>

psschwei · 2023-01-30T16:34:53Z

@mtreinish this should be ready for another review

mtreinish

This LGTM now, thanks for working on this and updating based on my earlier comments. Again, sorry for the slow reviews on this it kept falling off my radar because of vacation and then the rush in the lead up to the 0.23.0 release last week. I just left a quick inline suggestion on some docstring formatting fixes which I'll apply before tagging this for merge.

A quick follow up PR, if you're interested would be to make the same doc typo fixes in pass_manager_drawer but no pressure.

qiskit/visualization/pass_manager_visualization.py

mtreinish · 2023-02-01T00:07:58Z

qiskit/visualization/pass_manager_visualization.py

@@ -174,22 +274,7 @@ def pass_manager_drawer(pass_manager, filename=None, style=None, raw=False):
        graph.write_png(tmppath)  # pylint: disable=no-member

        image = Image.open(tmppath)
-        image = utils._trim(image)


This makes sense to me. The trim here isn't super necessary, IIRC (and I probably don't since it's been a while since I dove down these code paths) this was added primarily for the latex circuit visualization which ends up with a pillow image object for an entire pdf page and the image is trimmed to avoid excess whitespace. For these image files generated by graphviz we don't have this concern, so the trim call was mostly superfluous.

qiskit/visualization/pass_manager_visualization.py

This commit fixes some copy paste issues around the sphinx syntax for hyperlinks in the docstring for the new staged_pass_manager_drawer() function.

psschwei · 2023-02-01T17:37:00Z

A quick follow up PR, if you're interested would be to make the same doc typo fixes in pass_manager_drawer but no pressure.

Yeah, I can do that.

* adds support to PassManager drawer to display stages Signed-off-by: Paul S. Schweigert <paul@paulschweigert.com> * reviewer comments Signed-off-by: Paul S. Schweigert <paul@paulschweigert.com> * label, DRY code Signed-off-by: Paul S. Schweigert <paul@paulschweigert.com> * DRY output Signed-off-by: Paul S. Schweigert <paul@paulschweigert.com> * remove trim from passmanger draw output Signed-off-by: Paul S. Schweigert <paul@paulschweigert.com> * linting Signed-off-by: Paul S. Schweigert <paul@paulschweigert.com> * unused import Signed-off-by: Paul S. Schweigert <paul@paulschweigert.com> * Fix sphinx syntax for hyperlinks This commit fixes some copy paste issues around the sphinx syntax for hyperlinks in the docstring for the new staged_pass_manager_drawer() function. --------- Signed-off-by: Paul S. Schweigert <paul@paulschweigert.com> Co-authored-by: Matthew Treinish <mtreinish@kortar.org> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

psschwei requested review from a team and nonhermitian as code owners November 14, 2022 20:23

qiskit-bot added the Community PR PRs from contributors that are not 'members' of the Qiskit repo label Nov 14, 2022

psschwei force-pushed the staged-draw branch 4 times, most recently from a52f261 to 4b36fd1 Compare November 14, 2022 21:33

psschwei changed the title ~~[WIP] Adds support to PassManager drawer to display stages~~ Adds support to PassManager drawer to display stages Nov 14, 2022

1ucian0 self-assigned this Nov 19, 2022

javabster reviewed Dec 12, 2022

View reviewed changes

qiskit/visualization/pass_manager_visualization.py Outdated Show resolved Hide resolved

qiskit/visualization/pass_manager_visualization.py Show resolved Hide resolved

releasenotes/notes/staged-pass-manager-drawer-f1da0308895a30d9.yaml Outdated Show resolved Hide resolved

psschwei force-pushed the staged-draw branch from 4b36fd1 to 9091d39 Compare December 12, 2022 19:59

psschwei force-pushed the staged-draw branch from 9091d39 to ba56710 Compare January 11, 2023 20:43

mtreinish reviewed Jan 11, 2023

View reviewed changes

qiskit/visualization/pass_manager_visualization.py Outdated Show resolved Hide resolved

qiskit/visualization/pass_manager_visualization.py Outdated Show resolved Hide resolved

psschwei commented Jan 18, 2023

View reviewed changes

psschwei force-pushed the staged-draw branch from 72084e0 to 71e2ff4 Compare January 18, 2023 17:06

psschwei force-pushed the staged-draw branch from 71e2ff4 to 7d708f3 Compare January 18, 2023 18:38

psschwei commented Jan 18, 2023

View reviewed changes

psschwei added 6 commits January 20, 2023 09:29

adds support to PassManager drawer to display stages

2f5c351

Signed-off-by: Paul S. Schweigert <paul@paulschweigert.com>

reviewer comments

363b196

Signed-off-by: Paul S. Schweigert <paul@paulschweigert.com>

label, DRY code

f4c06ca

Signed-off-by: Paul S. Schweigert <paul@paulschweigert.com>

DRY output

695809d

Signed-off-by: Paul S. Schweigert <paul@paulschweigert.com>

remove trim from passmanger draw output

45577be

Signed-off-by: Paul S. Schweigert <paul@paulschweigert.com>

linting

fe1cf85

Signed-off-by: Paul S. Schweigert <paul@paulschweigert.com>

psschwei force-pushed the staged-draw branch from 7d708f3 to fe1cf85 Compare January 20, 2023 14:29

unused import

a3f6010

Signed-off-by: Paul S. Schweigert <paul@paulschweigert.com>

mtreinish assigned mtreinish and unassigned 1ucian0 Jan 31, 2023

mtreinish added this to the 0.24.0 milestone Jan 31, 2023

mtreinish added Changelog: New Feature Include in the "Added" section of the changelog mod: visualization qiskit.visualization labels Jan 31, 2023

mtreinish previously approved these changes Feb 1, 2023

View reviewed changes

mtreinish dismissed their stale review via 8a50711 February 1, 2023 00:12

Fix sphinx syntax for hyperlinks

8a50711

This commit fixes some copy paste issues around the sphinx syntax for hyperlinks in the docstring for the new staged_pass_manager_drawer() function.

mtreinish approved these changes Feb 1, 2023

View reviewed changes

mtreinish added the automerge label Feb 1, 2023

Merge branch 'main' into staged-draw

5879e69

mergify bot merged commit c9e0a42 into Qiskit:main Feb 1, 2023

psschwei deleted the staged-draw branch February 1, 2023 17:37

psschwei mentioned this pull request Feb 1, 2023

Fix doc typos in pass_manager_drawer #9512

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds support to PassManager drawer to display stages #9128

Adds support to PassManager drawer to display stages #9128

psschwei commented Nov 14, 2022

qiskit-bot commented Nov 14, 2022

coveralls commented Nov 15, 2022 •

edited

Loading

psschwei commented Nov 18, 2022

javabster left a comment

psschwei commented Dec 12, 2022

psschwei commented Jan 11, 2023

mtreinish left a comment

psschwei Jan 18, 2023

psschwei commented Jan 18, 2023 •

edited

Loading

psschwei Jan 18, 2023

mtreinish Feb 1, 2023

psschwei commented Jan 30, 2023

mtreinish left a comment

mtreinish Feb 1, 2023

psschwei commented Feb 1, 2023

Adds support to PassManager drawer to display stages #9128

Adds support to PassManager drawer to display stages #9128

Conversation

psschwei commented Nov 14, 2022

Summary

Details and comments

qiskit-bot commented Nov 14, 2022

coveralls commented Nov 15, 2022 • edited Loading

Pull Request Test Coverage Report for Build 4059859471

💛 - Coveralls

psschwei commented Nov 18, 2022

javabster left a comment

Choose a reason for hiding this comment

psschwei commented Dec 12, 2022

psschwei commented Jan 11, 2023

mtreinish left a comment

Choose a reason for hiding this comment

psschwei Jan 18, 2023

Choose a reason for hiding this comment

psschwei commented Jan 18, 2023 • edited Loading

psschwei Jan 18, 2023

Choose a reason for hiding this comment

mtreinish Feb 1, 2023

Choose a reason for hiding this comment

psschwei commented Jan 30, 2023

mtreinish left a comment

Choose a reason for hiding this comment

mtreinish Feb 1, 2023

Choose a reason for hiding this comment

psschwei commented Feb 1, 2023

coveralls commented Nov 15, 2022 •

edited

Loading

psschwei commented Jan 18, 2023 •

edited

Loading