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

Rename edge_list arg with edgelist to be compatible with networkx #341

Closed
t-imamichi opened this issue May 28, 2021 · 3 comments · Fixed by #344
Closed

Rename edge_list arg with edgelist to be compatible with networkx #341

t-imamichi opened this issue May 28, 2021 · 3 comments · Fixed by #344

Comments

@t-imamichi
Copy link
Member

What is the expected enhancement?

mpl_draw has an argument edge_list https://github.com/Qiskit/retworkx/blob/885f2de1588dc983591563f94b2ddcada443b3b2/retworkx/visualization/matplotlib.py#L101

The same argument has name edgelist in networkx
https://networkx.org/documentation/stable/reference/generated/networkx.drawing.nx_pylab.draw_networkx_edges.html

Related to qiskit-community/qiskit-optimization#150

@mtreinish
Copy link
Member

This was my fault when I ported the mpl drawer from networkx to retworkx in #304 I explicitly changed edgelist to edge_list since it seemed more consistent with retworkx's style (for example graph.edge_list()). But if you think it's better to be consistent with networkx's drawer I'll quickly put together a PR to rename the kwarg so we can make this change prior to the pending 0.9.0 release.

@t-imamichi
Copy link
Member Author

I see. edge_list makes more sense. I don't think you need to rename it; but it might be good to write a notice in the networkx users guide.

@mtreinish
Copy link
Member

Ok sure, I'll push another PR updating the networkx.rst guide to have a section about visualizations.

mtreinish added a commit to mtreinish/retworkx that referenced this issue May 28, 2021
This commit adds a section to the retworkx for networkx users guide on
visualizations. This is important especially for the ``mpl_draw``
function since it's mostly identical to the ``networkx_drawer`` function
but there are some key differences to keep in mind.

This commit also makes small change to the graphviz drawer, the name of
the function is renamed from `pydot_draw` to `graphviz_draw` and the
Python module is renamed from `pydot` to `graphviz`. This was done to
be more descriptive as graphviz is the backend not pydot. It also makes
it easier to migrate away from pydot in the future if we needed to since
the key piece is using graphviz not a particular python interface to it.

Fixes Qiskit#341
mtreinish added a commit to mtreinish/retworkx that referenced this issue May 28, 2021
This commit adds a section to the retworkx for networkx users guide on
visualizations. This is important especially for the ``mpl_draw``
function since it's mostly identical to the ``networkx_drawer`` function
but there are some key differences to keep in mind.

This commit also makes small change to the graphviz drawer, the name of
the function is renamed from `pydot_draw` to `graphviz_draw` and the
Python module is renamed from `pydot` to `graphviz`. This was done to
be more descriptive as graphviz is the backend not pydot. It also makes
it easier to migrate away from pydot in the future if we needed to since
the key piece is using graphviz not a particular python interface to it.

Fixes Qiskit#341
mtreinish added a commit that referenced this issue May 29, 2021
* Add visualization to retworkx for networkx users guide

This commit adds a section to the retworkx for networkx users guide on
visualizations. This is important especially for the ``mpl_draw``
function since it's mostly identical to the ``networkx_drawer`` function
but there are some key differences to keep in mind.

This commit also makes small change to the graphviz drawer, the name of
the function is renamed from `pydot_draw` to `graphviz_draw` and the
Python module is renamed from `pydot` to `graphviz`. This was done to
be more descriptive as graphviz is the backend not pydot. It also makes
it easier to migrate away from pydot in the future if we needed to since
the key piece is using graphviz not a particular python interface to it.

Fixes #341

* Update release note too

* Rename optional target to graphviz
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants