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

Adjust graphviz options for better layout with plot_gate_map() #12770

Merged
merged 12 commits into from
Sep 5, 2024

Conversation

mtreinish
Copy link
Member

Summary

This commit fixes an issue with the visualizations of some backends/coupling maps. The default neato settings worked well in most cases but for some graphs it was not generating a good layout. This tweaks the settings a bit so that layouts work with more classes of graphs.

Details and comments

This commit fixes an issue with the visualizations of some
backends/coupling maps. The default neato settings worked well in most
cases but for some graphs it was not generating a good layout. This
tweaks the settings a bit so that layouts work with more classes of
graphs.
@mtreinish mtreinish added stable backport potential The bug might be minimal and/or import enough to be port to stable Changelog: None Do not include in changelog labels Jul 12, 2024
@mtreinish mtreinish requested review from nonhermitian and a team as code owners July 12, 2024 13:42
@qiskit-bot
Copy link
Collaborator

One or more of the following people are relevant to this code:

@1ucian0 1ucian0 self-assigned this Jul 12, 2024
@1ucian0
Copy link
Member

1ucian0 commented Jul 12, 2024

ibm_fez
image

@mtreinish
Copy link
Member Author

Based on that image I'm wondering if we want to scale up the fontsize a bit?

@coveralls
Copy link

coveralls commented Jul 12, 2024

Pull Request Test Coverage Report for Build 10705646259

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 0 of 15 (0.0%) changed or added relevant lines in 1 file are covered.
  • 22 unchanged lines in 4 files lost coverage.
  • Overall coverage decreased (-0.03%) to 89.16%

Changes Missing Coverage Covered Lines Changed/Added Lines %
qiskit/visualization/gate_map.py 0 15 0.0%
Files with Coverage Reduction New Missed Lines %
qiskit/visualization/gate_map.py 1 6.69%
qiskit/transpiler/passes/synthesis/unitary_synthesis.py 2 88.43%
qiskit/synthesis/two_qubit/xx_decompose/decomposer.py 7 90.84%
crates/qasm2/src/parse.rs 12 97.15%
Totals Coverage Status
Change from base Build 10691830429: -0.03%
Covered Lines: 72526
Relevant Lines: 81344

💛 - Coveralls

@1ucian0
Copy link
Member

1ucian0 commented Jul 12, 2024

ibm_quebec

image

@mtreinish
Copy link
Member Author

That second example looks like we should condition:

https://github.com/Qiskit/qiskit/pull/12770/files#diff-ff70fb816cde3f453d870891f47ede8245855a9e156228201a53fd613ad00659R1096-R1100

on if not qubit_coordinates and only use these graph attributes if we're doing an algorithmic layout.

@1ucian0
Copy link
Member

1ucian0 commented Jul 13, 2024

In the last change, I changed to prism and I got the following results:

ibm_fez
image

ibm_cusconazca
image

ibm_torino
image

ibm_kyiv
image

If you are happy when them, I will update the image references and merge.

@mtreinish
Copy link
Member Author

If you are happy when them, I will update the image references and merge.

Those look fine to me, the ibm_torino one is maybe a bit less pretty than ibm_fez and ibm_cusconazca but it looks fine. So let's just go with this.

@1ucian0 1ucian0 added this to the 1.2.0 milestone Jul 19, 2024
@ElePT
Copy link
Contributor

ElePT commented Jul 30, 2024

It looks like the font size still needs some tweaking:

1_qubit_gate_map
5_qubit_gate_map
16_qubit_gate_map

@ElePT ElePT modified the milestones: 1.2.0, 1.2.1 Jul 31, 2024
@1ucian0
Copy link
Member

1ucian0 commented Sep 4, 2024

image
image
image
image
image
image
image
image
image

Copy link
Contributor

@Cryoris Cryoris 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 final tweaks @1ucian0 !

@Cryoris Cryoris added this pull request to the merge queue Sep 5, 2024
Merged via the queue into Qiskit:main with commit 52733ae Sep 5, 2024
15 checks passed
mergify bot pushed a commit that referenced this pull request Sep 5, 2024
* Adjust graphviz options for better layout with plot_gate_map()

This commit fixes an issue with the visualizations of some
backends/coupling maps. The default neato settings worked well in most
cases but for some graphs it was not generating a good layout. This
tweaks the settings a bit so that layouts work with more classes of
graphs.

* Don't set graph attributes if qubit_coordinates is set

* adjust sizes

* planar branching

* support for backendv1

* remove the formula

* new ref images

---------

Co-authored-by: Luciano Bello <bel@zurich.ibm.com>
Co-authored-by: Elena Peña Tapia <57907331+ElePT@users.noreply.github.com>
(cherry picked from commit 52733ae)
github-merge-queue bot pushed a commit that referenced this pull request Sep 5, 2024
… (#13089)

* Adjust graphviz options for better layout with plot_gate_map()

This commit fixes an issue with the visualizations of some
backends/coupling maps. The default neato settings worked well in most
cases but for some graphs it was not generating a good layout. This
tweaks the settings a bit so that layouts work with more classes of
graphs.

* Don't set graph attributes if qubit_coordinates is set

* adjust sizes

* planar branching

* support for backendv1

* remove the formula

* new ref images

---------

Co-authored-by: Luciano Bello <bel@zurich.ibm.com>
Co-authored-by: Elena Peña Tapia <57907331+ElePT@users.noreply.github.com>
(cherry picked from commit 52733ae)

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
@mtreinish mtreinish deleted the adjust-neato-settings branch September 5, 2024 17:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: None Do not include in changelog stable backport potential The bug might be minimal and/or import enough to be port to stable
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants