-
Notifications
You must be signed in to change notification settings - Fork 18
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
[unitaryhack] EGraph visualizations using Plotly 🎨 #88
[unitaryhack] EGraph visualizations using Plotly 🎨 #88
Conversation
Could you also share some screenshots or gifs of the new plots? 👀 |
Sure! |
@Avhijit-codeboy like the other UnitaryHack PRs, the same holds here
If you can deliver a final version later today (preferably before 16:00 Toronto time), I can check it and possibly mark it as approved. Please also update the PR description when finalised. |
Hi @soosub! As of now these are all the changes I can push today (getting too late here ). I didn't get time to check with a lot of test cases, but will do so soon. |
Thanks - I'll have a look rn 👀 |
Hey @Avhijit-codeboy, I just reviewed your proposal, and this is not yet what we were looking for. Unfortunately, this means that I can't approve it at the moment. Please address the following changes if you want to have the PR approved 👷🏼
Sadly, I can't promise we can review it again before the UnitaryHack ends 😿 However, if you want you can still continue with the PR so we can try to merge it later once we are satisfied with the result. I am not sure about the UH prize eligibility, but you would be listed as one of the contributors in a future release. Cheers |
No problem @soosub! I'll keep contributing to this issue and repo :) |
Cool! I'll go over your code sometime next week so I can help you out with some suggestions @Avhijit-codeboy |
color = egraph.nodes[point].get("color") or "black" | ||
else: | ||
color = "k" | ||
color = "black" |
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 would suggest not changing this because it's not relevant to the task at hand and might introduce unwanted changes/bugs
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.
But Plotly doesn't recognize 'k' as black.
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.
Ah, I see! Good catch. Does matplotlib
recognize `"black"'? In that case, your solution is perfect 👍🏼
EDIT: but hold up, I don't think you need this method for the plotly
drawing anyway? It acts on a Axis
object of matplotlib
(named ax
) so it should not come up in the new plotly
draw method. Am I missing something? @Avhijit-codeboy
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.
Need to check.
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 draw_Egraph_plotly()
method calls the _get_node_color()
method which returns a list of node colors which is then passed into the plotly's Scatter3D()
method. So I think changing the color name from k
to black
is required. And Matplotlib recognizes both k
and black
as black color.
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.
Okay I see, great solution in that case! 👍🏼 Sorry for the confusion
@@ -284,6 +285,138 @@ def draw_EGraph( | |||
|
|||
return fig, ax | |||
|
|||
@mpl.rc_context(plot_params) | |||
def draw_EGraph_3DScatterPlot( |
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.
@sduquemesa do you have any preference on how we will call this plot method? Should we use seperate functions/methods or should we pass "plotly" as kwarg to the EGraph draw method?
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.
@soosub, that is an interesting question: how to inform the objects which backend they should use to plot something. At first and for the scope of this PR is ok to use separate functions or methods. However, in the long run this shouldn't be the case and we must think of a way to integrate different plotting backends and abstract the logic (meaning no need to use different methods or functions to use one or other backend) from the users API.
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 would be in favour of renaming this function
def draw_EGraph_3DScatterPlot( | |
def draw_EGraph_plotly( |
and then add a key word argument to the draw method of EGraph
. Then EGraph.draw
would look something like
def draw(self, backend='matplotlib', **kwargs):
"""Draw the graph state with Matplotlib.
See flamingpy.utils.viz.draw_EGraph and flamingpy.utils.viz.draw_EGraph_plotly for more details.
"""
if backend == 'matplotlib':
from flamingpy.utils.viz import draw_EGraph
fig, ax = draw_EGraph(self, **kwargs)
return fig, ax
elif backend == 'plotly':
from flamingpy.utils.viz import draw_EGraph_plotly
fig = draw_EGraph_plotly(self, (**kwargs)
return fig
What do you you guys think @Avhijit-codeboy @sduquemesa? Avhijit, can you implement this?
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.
@soosub, I like it! What about something like this? just to remove redundant code
def draw(self, backend='matplotlib', **kwargs):
"""Draw the graph state with Matplotlib.
See flamingpy.utils.viz.draw_EGraph and flamingpy.utils.viz.draw_EGraph_plotly for more details.
"""
if backend == 'matplotlib':
from flamingpy.utils.viz import draw_EGraph
elif backend == 'plotly':
from flamingpy.utils.viz import draw_EGraph_plotly as draw_EGraph
fig, ax = draw_EGraph(self, **kwargs)
return fig, ax
By the way, @Avhijit-codeboy are this plots producing the expected output already?
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.
Hi @sduquemesa . This is absolutely implementable.
I haven't yet gone through the changes to see whether we are getting the expected output or not. Will do so shortly.
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.
@sduquemesa @Avhijit-codeboy this won't work because the structure of matplotlib
and plotly
is a bit different: plotly
only returns one object while for matplotlib
you usually work with two (fig, ax
)
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.
for edge in edge_list: | ||
#format: [beginning,ending,None] | ||
x_coords = [egraph_3D[edge[0]][0],egraph_3D[edge[1]][0],None] | ||
x_edges += x_coords | ||
|
||
y_coords = [egraph_3D[edge[0]][1],egraph_3D[edge[1]][1],None] | ||
y_edges += y_coords | ||
|
||
z_coords = [egraph_3D[edge[0]][2],egraph_3D[edge[1]][2],None] | ||
z_edges += z_coords |
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 suspect this can be reduced greatly. Can't you plot the edges more directly in a similar way to plotting the nodes as I suggested above?
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.
Will try.
for point in egraph.nodes: | ||
x,y,z = point | ||
color = _get_node_color(egraph, color_nodes, point) | ||
nodeColors.append(color) |
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.
maybe it's even beneficial to make a dataframe that contains all the info, i.e. x, y, an z coords plus colouring. I should consult with @sduquemesa for this. For very big codes this might be problematic, but then visualization is hard anyway.
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 pandas brings a lot of powerful tools that won't be used here. Plus, it is very memory-greedy. If this data is only relevant for plotting and won't be used in any other type of processing, I'd go against using pandas.
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.
cool, agreed
Returns: | ||
figure: Plotly Express figure. | ||
""" | ||
egraph_3D = nx.spring_layout(egraph,dim=3, seed=18) |
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 should not use the spring layout but just the coordinates that are given in the EGraph nodes
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.
see this comment below
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.
Alright.
Hi @Avhijit-codeboy, how are you doing? Just wanted to let you know that I'm available if you require any help or feedback 😌 We also recently set up two new FlamingPy support channels: a Slack channel and a discussion forum. Feel free to make use of those too to DM me for example 🤙🏼 |
Hi @soosub . Apologies for being inactive for some time. I got caught up with my job. But I'm having time this week and will work on integrating the changes. |
Hi @Avhijit-codeboy, I need the plotly visualization for a demo I'm working on so I decided worked on it a bit myself, see #103. I could take over from here, but feel free to collaborate with me on that one. In any case, I will include you in the list of contributors 👍🏼 |
Alright @soosub. Thanks! |
Context for changes
A better and more interactive way to visualize graph states.
Example usage and tests
Performance results justifying changes
< If you have improved a feature you should support it with some benchmarking and scaling results, plots, etc.; otherwise, mark N/A below. >
Workflow actions and tests
< In most cases, we need a set of unit tests that automatically and comprehensively test for newly added features. Discuss existing CI tests that cover the changes or any updates to the workflow here; otherwise, mark N/A below. >
Expected benefits and drawbacks
< Summarize your justifications for the change and its benefits. Is there any drawback that exists today with the changes or may occur in the future? You cannot place N/A here. >
Expected benefits:
Possible drawbacks:
Related Github issues
EGraph
visualizations more interactive using Plotly 🎨 #72Checklist and integration statements
My Python and C++ codes follow this project's coding and commenting styles as indicated by existing files. Precisely, the changes conform to given
black
,docformatter
andpylint
configurations.I have performed a self-review of these changes, checked my code (including for codefactor compliance), and corrected misspellings to the best of my capacity. I have synced this branch with others as required.
I have added context for corresponding changes in documentation and
README.md
as needed.I have added new workflow CI tests for corresponding changes, ensuring codecoverage is 95% or better, and these pass locally for me.
I have updated
CHANGELOG.md
following the template. I recognize that the developers may revisitCHANGELOG.md
and the versioning, and create a Special Release including my changes.