add constructor to main widget #258

vaniisgh · 2021-05-06T06:29:17Z

Hey,
This issue ( #94 ) looked interesting and beginner-friendly,
I just wanted to know if this was a good start.

github-actions · 2021-05-06T06:29:31Z

👈 Launch a binder notebook on branch vaniisgh/ipycytoscape/master

ianhi · 2021-05-06T16:43:05Z

Hi @vaniisgh thanks for taking this on! This looks like a good start but there are a few things to clean up.

TODO:

Add some examples documenting this
Add a docstring to __init__ with parameters section
Cleanup checking for string in the from_json method (I'll add an inline review) (edit I think you did this correctly and I was just reading the code too fast - my bad!)
Add a check for neo4j graphs
Change the name from graph_file to something more general.
- It's not necessarily a file that user will be passing in.
Make the formatter happy by running black .

Also looking at the test failures there are unfortunately several due to neo4j which aren't your fault. This makes it trickier to see if your are causing issues but you can look more closely by clicking on the details next to the failing test.

ipycytoscape/cytoscape.py

ianhi · 2021-05-06T16:44:54Z

ipycytoscape/cytoscape.py

        directed : bool
            If True all edges will be given 'directed' as a class if
            they do not already have it.
        """
+        if path.isfile(str(json_file)):


Does this fully protect us if the user passes in a dict?

ipycytoscape/cytoscape.py

ianhi · 2021-05-06T16:50:12Z

ipycytoscape/cytoscape.py


 from spectate import mvc
 from traitlets import TraitType, TraitError

+from pandas import DataFrame


We currently don't require pandas so this need to either be an optional import or the check below needs to check for pandas in the graph.__class__

I made a change to fix this, but I am not sure I passed the kwargs correctly to the add_graph_from_df correctly ... cold you please let me know what the correct way to do that would be? :)

ipycytoscape/cytoscape.py

vaniisgh · 2021-05-12T08:34:03Z

hey thank you so much for the feedback!
I tried to take the things you mentioned into account and made a few changes. Though, I still think I might need to revise my work a bit.
Also, I apologize forgot to rebase my work on the current master I hope I did it correctly now.

ianhi

This is looking good! I'll try look more closely (e.g. at the pandas import) in the coming days. For now I left a quick comment on the docstring and approved the github actions.

ianhi · 2021-05-12T15:10:48Z

ipycytoscape/cytoscape.py

+        - graph:
+          type: string|dict|pandas.DataFrame|networkx.Graph|neo4j|Graph|None
+          description: graph passes is checked to be of one of the declared
+                       types, and the corresponding object is added to the
+                       graph attribute of the DOM object.


Thanks for adding this! Can you update this to be in the numpydoc style as described here: https://numpydoc.readthedocs.io/en/latest/format.html#parameters

You can see an example here:

ipycytoscape/ipycytoscape/cytoscape.py

Lines 450 to 458 in b80a913

Parameters

----------

g : networkx graph

receives a generic NetworkX graph. more info in

https://networkx.github.io/documentation/

directed : bool

If true all edges will be given directed as class if

they do not already have it. Equivalent to adding

'directed' to the 'classes' attribute of edge.data for all edges

ianhi · 2021-05-14T19:46:26Z

ipycytoscape/cytoscape.py

+        if isinstance(graph, nx.Graph):
+            self.graph.add_graph_from_networkx(graph)
+        elif isinstance(graph, (dict, str)):
+            self.graph.add_graph_from_json(graph)
+        elif graph.__class__.__name__ == "DataFrame":
+            self.graph.add_graph_from_df(graph, **kwargs)


So this is the approach I've always taken to optional imports in the past, but looking at it now and reading around on the internet a bit I think this is probably not the best way. This is brittle to changes in class names and also to other objects that have a similar classname. I think a better approach would be to try to import pandas at the top level like this:

try: import pandas as pd except ImportError: pd = None

and then for this checks do:

elif pd and isinstance(graph, pd.DataFrame)

what do you think?

this seems like a good solution really. :) Though then should we do something similar for the neo4j graphs (because Graph is a far more common class name than DataFrame)?

Yup that's the best way forward!

ianhi · 2021-05-14T19:48:28Z

ipycytoscape/cytoscape.py

+        else:
+            if isinstance(graph, Graph):
+                self.graph = graph


any reason not to make these elifs at the same level?

What I was thinking was, that the graph.class.name is "Graph" for both objects, and I didn't want to add the optional import, I will recheck though.

vaniisgh · 2021-05-27T09:09:20Z

I think it should be better now :)

ianhi · 2021-05-27T16:51:32Z

Thanks for the ping! This slipped through the cracks for me - can look later

ipycytoscape/cytoscape.py

ianhi · 2021-06-02T04:04:31Z

ipycytoscape/cytoscape.py

+               Checked to be of one of the declared types, and graph object
+               corresponding to it is added as a Graph attribute to the
+               CytoscapeWidget DOM.


Suggested change

Checked to be of one of the declared types, and graph object

corresponding to it is added as a Graph attribute to the

CytoscapeWidget DOM.

The graph to initialize with. Equivalent to calling the appropriate ``CytoscapeWidget.graph.add_graph_from_` method.

Is this clearer? We maybe should stay away from using words like DOM as I suspect that isn't a super clear term to most python users.

vaniisgh · 2021-10-30T05:39:52Z

@ianhi should i close this ?

ianhi · 2021-11-27T01:48:00Z

Thank you @vaniisgh !! This is a great addition and we can always iterate more as needed.

ianhi · 2021-11-27T01:50:24Z

@ianhi should i close this ?

Definitely not that - I'm really sorry about the long periods of non-responsiveness that's totally on me. I really appreciate you coming and putting the effort to make a positive change. I can't promise to be perfect, but I can promise that I will at least try to be more honest myself about what I can claim to do. Also don't be scared to ping more and more often (though ideally course you shouldn't have to :/ )

ianhi · 2021-11-27T02:28:50Z

This should be live in version 1.3.0 soon!

add constructor to main widget (cytoscape#94)

7502e8d

ianhi reviewed May 6, 2021

View reviewed changes

ipycytoscape/cytoscape.py Outdated Show resolved Hide resolved

ianhi reviewed May 6, 2021

View reviewed changes

ipycytoscape/cytoscape.py Outdated Show resolved Hide resolved

ianhi reviewed May 6, 2021

View reviewed changes

ipycytoscape/cytoscape.py Outdated Show resolved Hide resolved

ianhi linked an issue May 6, 2021 that may be closed by this pull request

Add a constructor in the main widget #94

Closed

vaniisgh added 2 commits May 12, 2021 13:50

add examples and docstrings

459979b

Merge branch 'master' of https://github.com/QuantStack/ipycytoscape

17b633b

ianhi reviewed May 12, 2021

View reviewed changes

ianhi reviewed May 14, 2021

View reviewed changes

vaniisgh added 2 commits May 19, 2021 12:50

fix imports & docstring

728c500

fix typo in docstrings

1f0960c

ianhi reviewed May 27, 2021

View reviewed changes

ipycytoscape/cytoscape.py Outdated Show resolved Hide resolved

ianhi added 2 commits May 27, 2021 13:16

fix py2neo optional import

01b7827

add neo4j requirements to examples optional install

880fcea

ianhi reviewed Jun 2, 2021

View reviewed changes

ipycytoscape/cytoscape.py Outdated Show resolved Hide resolved

ianhi reviewed Jun 2, 2021

View reviewed changes

update docstring

fb2a0d5

ianhi merged commit 79a4000 into cytoscape:master Nov 27, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add constructor to main widget #258

add constructor to main widget #258

vaniisgh commented May 6, 2021

github-actions bot commented May 6, 2021

ianhi commented May 6, 2021 •

edited

ianhi May 6, 2021

ianhi May 6, 2021

vaniisgh May 12, 2021

vaniisgh commented May 12, 2021 •

edited

ianhi left a comment •

edited

ianhi May 12, 2021

ianhi May 14, 2021

vaniisgh May 15, 2021 •

edited

ianhi May 16, 2021

ianhi May 14, 2021

vaniisgh May 15, 2021

vaniisgh commented May 27, 2021

ianhi commented May 27, 2021

ianhi Jun 2, 2021

vaniisgh commented Oct 30, 2021

ianhi commented Nov 27, 2021

ianhi commented Nov 27, 2021

ianhi commented Nov 27, 2021

	Parameters
	----------
	g : networkx graph
	receives a generic NetworkX graph. more info in
	https://networkx.github.io/documentation/
	directed : bool
	If true all edges will be given directed as class if
	they do not already have it. Equivalent to adding
	'directed' to the 'classes' attribute of edge.data for all edges

add constructor to main widget #258

add constructor to main widget #258

Conversation

vaniisgh commented May 6, 2021

github-actions bot commented May 6, 2021

ianhi commented May 6, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vaniisgh commented May 12, 2021 • edited

ianhi left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vaniisgh May 15, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vaniisgh commented May 27, 2021

ianhi commented May 27, 2021

Choose a reason for hiding this comment

vaniisgh commented Oct 30, 2021

ianhi commented Nov 27, 2021

ianhi commented Nov 27, 2021

ianhi commented Nov 27, 2021

ianhi commented May 6, 2021 •

edited

vaniisgh commented May 12, 2021 •

edited

ianhi left a comment •

edited

vaniisgh May 15, 2021 •

edited