-
Notifications
You must be signed in to change notification settings - Fork 167
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
Add matplotlib based drawer #304
Conversation
This commit adds a a new python visualization module which contains a matplotlib drawer for retworkx graphs. This first iteration of the visualization function is porting networkx's equivalent functionality. For now the api is basically identical to networkx's functionality except it takes a retworkx graph object instead of a networkx graph object. However, in follow-on commits to this branch the API will be adapted to be more retworkx native (i.e. using callbacks to generate labels from nodes and edges, etc). To implement this a basic random_layout function is added to retworkx and a corresponding 2D layout type Pos2DMapping which is used to return the layouts efficiently (instead of creating a dict). This is still a rough draft and before this merges into master things will be updated. Fixes Qiskit#298
Just to show the basics here, running: import retworkx
from retworkx import visualization
import matplotlib.pyplot as plt
g = retworkx.directed_gnp_random_graph(10, .9)
fig = plt.figure()
ax = fig.gca()
visualization.mpl_draw(g, ax=ax)
fig.savefig('test.png', dpi=600) yields: There is still a lot of work to do here, but the basics work for now. |
Pull Request Test Coverage Report for Build 853089054
💛 - Coveralls |
This commit adds a new function random_layout() which is used to generate a random layout for a graph that can be used in visualization. This is necessary for building a matplotlib drawer (issue Qiskit#298 and a first draft of the implementation Qiskit#304). To make the function more efficient it also adds a new custom return type Pos2DMapping which is used to build an imutable readonly dict compatible result container for the output type from this function. Related to Qiskit#280
I've split out the layout functionality into #305 |
This commit adds a new function random_layout() which is used to generate a random layout for a graph that can be used in visualization. This is necessary for building a matplotlib drawer (issue Qiskit#298 and a first draft of the implementation Qiskit#304). To make the function more efficient it also adds a new custom return type Pos2DMapping which is used to build an imutable readonly dict compatible result container for the output type from this function. Related to Qiskit#280
This commit starts the process of reorganizing the matplotlib drawer into the final form. It flattens the api so there is a single public api entrypoint, mpl_draw() which exposes all the drawer options and returns a figure (if not in interactive mode). This is the first module and function in the visualization subpackage. In the future we will add some other ones built around graph.to_dot() that use pydot (so we avoid the boilerplate drawer code everywhere).
* Add random_layout function and Pos2DMapping This commit adds a new function random_layout() which is used to generate a random layout for a graph that can be used in visualization. This is necessary for building a matplotlib drawer (issue #298 and a first draft of the implementation #304). To make the function more efficient it also adds a new custom return type Pos2DMapping which is used to build an imutable readonly dict compatible result container for the output type from this function. Related to #280 * Add release note * Update doc organization * Apply suggestions from code review Co-authored-by: Jielun (Chris) Chen <[email protected]> * Update releasenotes/notes/add-random-layout-c1c2751be971e5d0.yaml Co-authored-by: Nahum Rosa Cruz Sa <[email protected]> * Update releasenotes/notes/add-random-layout-c1c2751be971e5d0.yaml Co-authored-by: georgios-ts <[email protected]> * Use [f64; 2] for center type This commit changes the input type of the center option to be a fixed 2 element array instead of a tuple. This still works as a tuple for the python interface but has a defined contiguous memory layout in rust. It also is consistent with the type for elements in the position mapping. Co-authored-by: Jielun (Chris) Chen <[email protected]> Co-authored-by: Nahum Rosa Cruz Sa <[email protected]> Co-authored-by: georgios-ts <[email protected]>
import matplotlib.pyplot as plt | ||
|
||
mpl.use("PS") | ||
plt.rcParams["text.usetex"] = False |
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.
Why it is needed to disable usetex?
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.
It's mostly for reproducibility in the tests, matplotlib's usetex mode relies on external latex installations to run: https://matplotlib.org/stable/tutorials/text/usetex.html and even if present can cause different behavior on different platforms or latex distributions. So just simplicity it makes sense to disable it here so things run consistently between different systems (although I don't think it should come up in any of the tests).
That being said this line was just borrowed from networkx's equivalent drawer test module: https://github.com/networkx/networkx/blob/ead0e65bda59862e329f2e6f1da47919c6b07ca9/networkx/drawing/tests/test_pylab.py#L9
# This is the only case handled differently from matplotlib | ||
if ( | ||
np.iterable(edge_color) | ||
and (len(edge_color) == len(edge_pos)) |
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.
Could it be possible for edge_color
to be a dict?
My reasoning is that we could have a default color and just change some of the nodes that we want to change using a dictionary with edge_pos
as key values. Is that reasonable?
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.
Well edge_pos
isn't user facing (it's just internal array of node positions for the nodes of an edge), but I think I see what your getting at here. The only way we can do different colors for a subset of edges now is multiple calls to mpl_draw
with a subset of nodes and edges (the tests show examples of this). In networkx this kind of thing is handled normally by calling draw_edges()
separately, but for this PR I opted for a single function for simplicity. This however causes some tension for more custom things like different colors for specific edges or nodes. I think there are 3 ways to address this:
- Make the internal
draw_nodes
,draw_edges
, etc functions part of the user facing api like networkx does. - Add support for dicts/mappings as an input to
edge_colors
,node_colors
,node_shape
(and maybenode_size
) so that these properties can be set per node and per edge. - Add callback function support for
edge_colors
,node_colors
, etc so that when we evaluate colors we can just call the user provided function to get a color string (or whatever other property.
None of these options are mutually exclusive so we can do any combination of them. How do you think we should proceed here?
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.
The most natural solution, at least for me, is the first one. However, exposing those functions may lead to some trouble. For the long term, I think the second solution would be better. What you think about doing both 1 and 2?
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 was on the fence about doing 1, but I agree it seems like it makes the most sense for user expectations. I think it'll be good as a followup (either before or after release). Same for 2, we can do that in a follow up. I'm going to merge this now (with your review) I'll open an issue about this after merging.
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.
LGTM. Overall it looks great, and the tests are excellent.
The only comment is to make the drawer function more customizable regarding the coloring of the graph, but this could be a future improvement.
Since Qiskit#304 merged building the retworkx documentation has required that matplotlib be installed for generating the matplotlib visualizations in the documentation of `mpl_draw()`. However that PR neglected to add it to the read the docs conda environment configuration. This commit corrects this oversight to hopefully fix the documentation builds on read the docs.
* Add matplotlib to read the docs builds Since #304 merged building the retworkx documentation has required that matplotlib be installed for generating the matplotlib visualizations in the documentation of `mpl_draw()`. However that PR neglected to add it to the read the docs conda environment configuration. This commit corrects this oversight to hopefully fix the documentation builds on read the docs. * Fix docs theme formatting This commit fixes some docs theme formatting issues to make sure the compiled HTML docs are correctly formatted. It mainly adds missing template files and configuration so the qiskit-sphinx-theme package has all the necessary details to correctly render. * Update copyright string
This commit adds a a new python visualization module which contains a
matplotlib drawer for retworkx graphs. This first iteration of the
visualization function is porting networkx's equivalent functionality.
For now the api is basically identical to networkx's functionality
except it takes a retworkx graph object instead of a networkx graph
object. However, in follow-on commits to this branch the API will be
adapted to be more retworkx native (i.e. using callbacks to generate
labels from nodes and edges, etc).
Fixes #298
TODO:
retworkx
from function namesFigure
object