-
Notifications
You must be signed in to change notification settings - Fork 155
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 mpl_draw() to fix multigraph plots #1204
Conversation
Pull Request Test Coverage Report for Build 9448236772Details
💛 - Coveralls |
@mtreinish one major hiccup in the implementation is that the position of the edges is calculated on the plot in the |
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 think we need to work on the loop condition. You'll also need to add a release note (https://github.com/Qiskit/rustworkx/blob/main/CONTRIBUTING.md#release-notes)
|
||
reverse_edge = np.concatenate(([dst], [src])) | ||
for edge in edge_pos: # the loop can be optimized | ||
if bool(np.sum(np.all(np.equal(edge, reverse_edge)))): |
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 think there are easier and simpler ways to write this. I don't quite understand why we are using NumPy for this check either
Co-authored-by: Ivan Carvalho <8753214+IvanIsCoding@users.noreply.github.com>
@IvanIsCoding I have not tried to align the labels vertically as, following the logic of |
# radius of edges | ||
reverse_edge = [dst, src] | ||
if ( | ||
len(np.where(np.all(edge_pos == reverse_edge, axis=(1, 2)))[0]) != 0 |
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.
So this scales quadratically on the number of edges. O(E^2)
is not ideal, if we plot a dense graph this is going to be a problem. You need to rewrite this condition using a set
or something similar instead of naively looking through all the edges
other: | ||
- | | ||
The radius of the edges of self-loops in multigraphs is set to `0.25`. | ||
The edge labels are offset accordingly. |
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.
You don't need the other section
- | | ||
Fixes the plots of multigraphs using the `mpl_draw()` function. Refer to | ||
`#12345 <https://github.com/Qiskit/rustworkx/issues/774>` for more | ||
details. |
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 release notes would be better if you actually drew a multigraph with mpl_draw
as an example. You can use
mpl_draw(random_graph) |
Co-authored-by: Ivan Carvalho <8753214+IvanIsCoding@users.noreply.github.com>
Fixes #774.
Updates:
draw_edges()
: setrad=0.25
for looped nodes.draw_edge_labels()
: Modify label offsets for looped nodes.