-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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 plot_gate_map() family to leverage graphviz for visualization #10208
Conversation
Update the gate_map.py to migrate the visualization modules from matplotlib to rustworkx.graphviz
Updated tests for the modified gate_map.py file
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:
|
Rustworkx is a requirement for qiskit, so it will always be installed when using qiskit. You will need a an optional check on
The easiest way to implement this is to use
TBH, the output of planar layout isn't really suitable for this use case. At the time I added that comment to the code I assumed it was without actually looking (based solely on the name). But this isn't an issue if you're doing a matplotlib based drawing the only real option is to use The thing which might be more interesting to try (and definitely higher quality) instead of rustworkx's |
@mtreinish I have a question about the tests: I have modified the checks, but need to update the images with which the graphs are compared. Should I wait for a review before changing the figures for the tests? |
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 is a great start, thanks for looking at this. I just did a quick high level pass to start and left some comments inline.
releasenotes/notes/update-gate_map-visualizations-6ea907a0502fdc1a.yaml
Outdated
Show resolved
Hide resolved
releasenotes/notes/update-gate_map-visualizations-6ea907a0502fdc1a.yaml
Outdated
Show resolved
Hide resolved
You can go ahead an update them now, in the worst case if we change the formatting of the output visualizations at all we can easily just regenerate them again to account for any future changes. |
@mtreinish thank you for the thorough review of my PR. I have modified my code to reuse the exisiting code. What really helped was using the One issue that may need to be addressed is using a better function than @nonhermitian, as this issue is a part of UnitaryHack, can we try to have the PR merged by June 13? 😄 I realise that it's a close call, but I'll be available at your discretion. |
Test file updated so that all tests can be passed.
This is the core issue of #9031 that the algorithmic layout functions in rustworkx don't scale for large graphs. This is why I used My concern with just using I think the best way to test this issue is to build a couple of large qubit count fake backends and test them to see what the graphs look like. For example using import numpy as np
import rustworkx as rx
from qiskit import transpile, QuantumCircuit
from qiskit.providers import BackendV2, Options
from qiskit.transpiler import Target, InstructionProperties
from qiskit.circuit.library import XGate, SXGate, RZGate, CZGate
from qiskit.circuit import Measure, Delay, Parameter, IfElseOp
from qiskit.visualization import plot_gate_map
class FakeMultiChip(BackendV2):
"""Fake multi chip backend."""
def __init__(self, distance=3, number_of_chips=3):
"""Instantiate a new fake multi chip backend.
Args:
distance (int): The heavy hex code distance to use for each chips'
coupling map. This number **must** be odd. The distance relates
to the number of qubits by:
:math:`n = \\frac{5d^2 - 2d - 1}{2}` where :math:`n` is the
number of qubits and :math:`d` is the ``distance``
number_of_chips (int): The number of chips to have in the multichip backend
each chip will be a heavy hex graph of ``distance`` code distance.
"""
super().__init__(name='multi_chip')
graph = rx.generators.directed_heavy_hex_graph(distance, bidirectional=False)
num_qubits = len(graph) * number_of_chips
rng = np.random.default_rng(seed=12345678942)
rz_props = {}
x_props = {}
sx_props = {}
measure_props = {}
delay_props = {}
self._target = Target("Fake multi-chip backend", num_qubits=num_qubits)
for i in range(num_qubits):
qarg = (i,)
rz_props[qarg] = InstructionProperties(error=0.0, duration=0.0)
x_props[qarg] = InstructionProperties(
error=rng.uniform(1e-6, 1e-4), duration=rng.uniform(1e-8, 9e-7)
)
sx_props[qarg] = InstructionProperties(
error=rng.uniform(1e-6, 1e-4), duration=rng.uniform(1e-8, 9e-7)
)
measure_props[qarg] = InstructionProperties(
error=rng.uniform(1e-3, 1e-1), duration=rng.uniform(1e-8, 9e-7)
)
delay_props[qarg] = None
self._target.add_instruction(XGate(), x_props)
self._target.add_instruction(SXGate(), sx_props)
self._target.add_instruction(RZGate(Parameter("theta")), rz_props)
self._target.add_instruction(Measure(), measure_props)
self._target.add_instruction(Delay(Parameter("t")), delay_props)
cz_props = {}
for i in range(number_of_chips):
for root_edge in graph.edge_list():
offset = i * len(graph)
edge = (root_edge[0] + offset, root_edge[1] + offset)
cz_props[edge] = InstructionProperties(
error=rng.uniform(1e-5, 5e-3), duration=rng.uniform(1e-8, 9e-7)
)
self._target.add_instruction(CZGate(), cz_props)
self._target.add_instruction(IfElseOp, name="if_else")
@property
def target(self):
return self._target
@property
def max_circuits(self):
return None
@classmethod
def _default_options(cls):
return Options(shots=1024)
def run(self, circuit, **kwargs):
raise NotImplementedError("Lasciate ogne speranza, voi ch'intrate")
class FakeGiantRing(BackendV2):
"""Fake multi chip backend."""
def __init__(self):
"""Instantiate a new 1k qubit ring backend."""
super().__init__(name='multi_chip')
graph = rx.generators.directed_cycle_graph(1000)
num_qubits = len(graph)
rng = np.random.default_rng(seed=12345678942)
rz_props = {}
x_props = {}
sx_props = {}
measure_props = {}
delay_props = {}
self._target = Target("Fake multi-chip backend", num_qubits=num_qubits)
for i in range(num_qubits):
qarg = (i,)
rz_props[qarg] = InstructionProperties(error=0.0, duration=0.0)
x_props[qarg] = InstructionProperties(
error=rng.uniform(1e-6, 1e-4), duration=rng.uniform(1e-8, 9e-7)
)
sx_props[qarg] = InstructionProperties(
error=rng.uniform(1e-6, 1e-4), duration=rng.uniform(1e-8, 9e-7)
)
measure_props[qarg] = InstructionProperties(
error=rng.uniform(1e-3, 1e-1), duration=rng.uniform(1e-8, 9e-7)
)
delay_props[qarg] = None
self._target.add_instruction(XGate(), x_props)
self._target.add_instruction(SXGate(), sx_props)
self._target.add_instruction(RZGate(Parameter("theta")), rz_props)
self._target.add_instruction(Measure(), measure_props)
self._target.add_instruction(Delay(Parameter("t")), delay_props)
cz_props = {}
for root_edge in graph.edge_list():
edge = (root_edge[0], root_edge[1])
cz_props[edge] = InstructionProperties(
error=rng.uniform(1e-5, 5e-3), duration=rng.uniform(1e-8, 9e-7)
)
self._target.add_instruction(CZGate(), cz_props)
self._target.add_instruction(IfElseOp, name="if_else")
@property
def target(self):
return self._target
@property
def max_circuits(self):
return None
@classmethod
def _default_options(cls):
return Options(shots=1024)
def run(self, circuit, **kwargs):
raise NotImplementedError("Lasciate ogne speranza, voi ch'intrate")
if __name__ == "__main__":
backend_multichip = FakeMultiChip(15, 5)
backend_ring = FakeGiantRing()
plot_gate_map(backend_multichip).savefig('giant_multichip.png')
plot_gate_map(backend_ring).savefig('ring.png') this is what you get out today: which is less than useful :) While this output is with the current |
Out of curiosity I pulled the PR branch locally and ran the script I posted above and this was the output: which is better than the layout from the manual mpl code before, but it really shows what I was talking about with the rustworkx |
Finally showing the output from graphviz using the basic script I put in #9031:
|
In that case,
In that case, should I drop the |
I've tested the new font size on a test code. Does this work? |
Yeah, I think that should be fine, it's about legibility, not necessarily matching the exact image as before. As long as the numbers are legible and not getting clipped by the node's circle it should be fine. |
@maxwell04-wq: just so you know, you don't need to keep your branch up to date with |
Pull Request Test Coverage Report for Build 6266203618
💛 - 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.
I just made some small tweaks to the formatting and fixed the font size issues. Also I updated the reference images in the visual comparison tests. I think this is ready to go now. Thanks a whole lot for doing this and sticking with it through all the updates. This project turned out to be a bit more finicky than it first seemed.
I think we'll probably be dealing with small formatting details for a while after making this change, but the gains from using a proper graph visualization engine are going to be a net win.
Summary
Fixes #9031
Details and comments
qiskit-terra/qiskit/visualization/gate_map.py
to leverage graphviz plotting for gate mapsplot_gate_map
plot_coupling_map
plot_circuit_layout
plot_error_map
rx
andmatplotlib
forplot_error_map
, uses onlyrx.graphvix_draw
for all other functions.qiskit.utils.optionals
does not have aHAS_RUSTWORKX
check. This can be added to ensure that rustworkx is installed prior to running thegate_map.py
modules.graphviz-draw()
function does not provide the choice to draw directed or undirected graphs. I have left theplot_directed
parameter in the functions, but as of now it does not affect graph plots.planar_layout()
is not yet available in rustwokx, the graphs have been plotted usingspring_layout()
. This can be modified in future versions.