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

Plkmapper #74

Closed
wants to merge 14 commits into from
Closed

Plkmapper #74

wants to merge 14 commits into from

Conversation

empet
Copy link
Contributor

@empet empet commented Mar 25, 2018

Here are my commits on kmapper and visuals. The module plotlyviz is new. It requires igraph and plotly.
The folder plotly-examples contains jupyter notebooks that illustrate how to generate plotly plots of kmapper graphs.
The notebook I posted as a gist one week ago is no more valid, because meanwhile I changed the function plotly_graph.

In the definition of the class KeplerMapper I modified only the visualize method:

  • When it is called with path_html = None, then it will return the dict json_graph and the string meta;
    the string meta is in this case defined within the new function format_meta_plotly.

In visuals the following changes were performed:

  • defined the function format_meta_plotly
  • the function dict_to_json has a new argument, path_html. When path_html is None this function
    returns the dict json_dict, otherwise it works as before.
    -_format_tooltip has also the argument path_html. When it is None the tooltip is formatted following plotly rules;

I didn't create a version for color distribution, because I associated a colorbar to the graph plot.
(the colorbar points out how the values from node['color'] are mapped to the Plotly colorscale).
Here I used a continuous colorscale, while the color_function_distribution maps values to a set of discrete colors (am I right?)

Also I haven't defined yet a plotly version for _format_cluster_statistics.

The new module plotlyviz contains three functions:

  • get_plotly_data() that returns the lists of x, y-coordinates of the graph nodes (Xnodes, Ynodes),
    and the x, y--coordinates of the end nodes of graph edges (Xedges, Yedges);
  • plotly_graph() that defines a graph as an igraph.Graph() instance, associates a graph layout,
    i.e. a planar position for each node, using either the Kamada-Kawai or Fruchterman Reingold layout algorithm.
    To each node one associates the attributes size and color. Color is a list of numerical values computed by a custom or default color_function.

All graph features are used to define the dicts, edges_trace, nodes_trace. The list of these dicts represents the Plotly data to plot
the graph.

  • The function plot_layout returns the dict that defines the Plotly plot layout.

@sauln sauln self-requested a review March 27, 2018 22:55
Copy link
Member

@sauln sauln left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, it looks good. There are a few minor things that need fixing and then we can merge the PR.

  • The README hasn't been changed, undo these wholesale changes.
  • Remove .ipynb_checkpoints from commit.
  • Run all the code through a linter.
  • Fix the failing test.

@@ -0,0 +1,220 @@
{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove these checkpoints files.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed the .ipynb_checkpoints.
README.md has been changed, because I added igraph and plotly to Kepler-Mapper requirements.

@@ -0,0 +1,116 @@
import igraph as ig
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please run this file through a Python linter. There are many PEP8 issues.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please format the comments for each function using Numpy comment specs

https://github.com/numpy/numpy/blob/master/doc/HOWTO_DOCUMENT.rst.txt

json_dict["nodes"].append(n)
for i, (node_id, linked_node_ids) in enumerate(graph["links"].items()):
for linked_node_id in linked_node_ids:
l = {"source": node_id_to_num[node_id],
"target": node_id_to_num[linked_node_id],
"width": _size_link_width(graph, node_id, linked_node_id)}
json_dict["links"].append(l)
if path_html is None:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a fan of multiple return statements. Can we consolidate these? Maybe seomthing like

return json_dict if path_html is none else json.dumps(json_dict)

@@ -119,8 +137,7 @@ def _format_cluster_statistics(member_ids, inverse_X, inverse_X_names):

stats = sorted([(s, f, i, c, a, v) for s, f, i, c, a, v in zip(std_m,
inverse_X_names,
np.mean(
inverse_X, axis=0),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

undo.

@@ -0,0 +1,221 @@
{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All of these notebooks are the same as the normal example, right?

Let's put them inside the normal examples directory and name them something parallel to what's in there, like cat.ipynb

So then the cat example directory will have files

cat_reference.csv
cat.py
cat.ipynb
...

@pep8speaks
Copy link
Contributor

pep8speaks commented May 2, 2018

Hello @empet, Thank you for updating !

Line 116:17: E722 do not use bare except'
Line 120:9: E722 do not use bare except'
Line 447:31: E241 multiple spaces after ','
Line 541:13: E265 block comment should start with '# '

Line 44:42: E272 multiple spaces before keyword
Line 74:13: E741 ambiguous variable name 'l'
Line 80:1: E302 expected 2 blank lines, found 1
Line 143:49: E712 comparison to True should be 'if cond is True:' or 'if cond:'
Line 144:49: E712 comparison to False should be 'if cond is False:' or 'if not cond:'
Line 204:11: E271 multiple spaces after keyword
Line 214:47: E261 at least two spaces before inline comment
Line 214:48: E262 inline comment should start with '# '

Comment last updated on May 02, 2018 at 20:22 Hours UTC

@sauln
Copy link
Member

sauln commented May 10, 2018

Thank you @empet for this work. I really like what's going on here and think having easier plotting for Jupyter would be great.

I've been overhauling some of the D3 code and I think we should put some more thought into how we integrate these features. Ideally, we could trim the interface down considerably and make it more natural to develop code for both the D3 output and the plotly output.

Maybe it would be better if the user doesn't call visualize when using plotly. Instead, they would only call pl.plotly_graph and pl.plot_layout. We could also move visualize out of KeplerMapper() all together and into it's own class. This could make it a bit more natural to switch which visualization method we use.

For plotly, is it normal for users to always run the init_notebook_mode(connected=True) boiler plate all the time? Can we wrap all of these plotly details and just supply an function to produce the plot?

@empet
Copy link
Contributor Author

empet commented May 13, 2018

@sauln

  • We can define an independent option to plot the graph via Plotly, and avoid using visualize .

  • plotly.offline must be run in each notebook, because it injects the plotly.js source files into that notebook. Another option is to send the plot to Plotly cloud in the user's online account, via:

import plotly.plotly as py
py.sign_in('username', 'api_key')
py.iplot(fig, filename='my-kmapper-graph')

or his company's internal Plotly Enterprise.

When you decide how to split the two cases, let me know, and I'll perform the necessary changes.

@sauln
Copy link
Member

sauln commented Jun 19, 2018

Hi @empet, sorry for the long delay. I've been mulling over the best way to deal with multiple visualization interfaces and think I have something.

How about we employ the strategy pattern here and build separate interfaces for each plotting utility as a class. This would end up looking similar to how the Cover and Nerve work. I image there would be a few different strategies, plotly, html, htmlJupyter, and these could then be expanded to incorporate other methods.

What are your thoughts on this?

The API might look like

km.visualize(graph, meta_data, **options, visualizer=km.VizPlotly())

or

km.visualize(graph, meta_data, **options, visualizer=km.DefaultHTML())

@empet
Copy link
Contributor Author

empet commented Jun 19, 2018

Hi @sauln, I'll work on it during the next week. Thanks for suggestion!

@sauln
Copy link
Member

sauln commented Jun 19, 2018

Great!

I’ll also have some time the next week, so please push to this PR as often as possible and I’ll try to contribute as much as I can. I expect this will be a bit of an overhaul, so us putting some thought into it will be important.

Thank you again for this! I’m excited for easy viz possibilities.

@empet
Copy link
Contributor Author

empet commented Jul 1, 2018

Hi @sauln,

Recently a major Plotly release was announced. That's why I have to postpone working on Plotly visualize, until I experiment the new great features, in order to be able to use them in my work. Thanks for your patience and understanding.

@empet
Copy link
Contributor Author

empet commented Jul 27, 2018

@sauln The code in plkmapper works only with Plotly version <3.0.0. Since 3.0.0 was a major release a few classes were removed or changed, and a lot of new features were introduced. Only after 2 weeks is recommended to use a much more stable version to rewrite the code as you requested. The author of these changes in plotly.py is still working on a few issues.

@sauln
Copy link
Member

sauln commented Jul 27, 2018

@empet, thanks for the update! I look forward to seeing what the new version of plotly can do and incorporating it into km.

@sauln
Copy link
Member

sauln commented Sep 11, 2018

Hi @empet, we're hoping to submit km to JoSS in the coming weeks and I would like to include your plotly visualization system in the submission. Would it be possible to get a PR in before then? How could I help? Please feel free to reach out via twitter dm or email (nat@saulgill.com) if you'd like to talk a bit about this.

Thank you!

@empet
Copy link
Contributor Author

empet commented Sep 11, 2018

Hi @sauln,
I have only to insert details in Readme and I'll open the new PR in the next 20 hours. :)

@empet empet closed this Sep 12, 2018
@empet empet deleted the plkmapper branch September 12, 2018 13:54
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 this pull request may close these issues.

3 participants