-
-
Notifications
You must be signed in to change notification settings - Fork 78
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
Fix plot_complexes
#1138
Fix plot_complexes
#1138
Conversation
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.
Can you add some tests to cover the bug with respect to the ordering, including in the case of multiple edges?
for (i,e) in enumerate(edgelist) | ||
fwd = findall(==(e), edgelist); rev = findall(==(reverse(e)), edgelist) | ||
distances[i] != 0 && continue | ||
distances[fwd] = collect(0:inc:inc*(length(fwd)-1)) | ||
distances[rev] = collect(inc:inc:inc*(length(rev))) |
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 seems like a very inefficient way to do this? Maybe build a mapping from unique edges to vectors of their indices in edgelist as part of the MultiGraph?
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.
Wrote a more efficient new way (that also correctly does the curves for multiple fwd/rev edges). One pass through the array and then one pass through the dict. Think it should be the same as storing the dict, since we construct a new MultiGraphWrap everytime we call plot_network
and plot_complexes
anyway
Co-authored-by: Sam Isaacson <[email protected]>
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.
@vyudu, feel free to merge if tests pass and the docs build.
Fix for #1135. Should also resolve the documentation build issue in #1130.