adds wrappers for networkx so you can modify the graph from within mesa#617
Conversation
|
This looks good! I'm adding a few comments on specific lines. [deleted an idea that upon slightly more thought wasn't very helpful] PS: Congratulations on your first pull request! For future reference -- if you want to link directly to an issue, you can type the hash sign to get a list of issues, and if you write eg #616 GitHub will create the link automatically. |
|
@dmasad is there a reason that this wasn't merged in already? |
| G.nodes[node_id]['agent'] = list() | ||
| self.G.nodes[node_id]['agent'] = list() | ||
|
|
||
| def add_nodes(self, node_ids): |
There was a problem hiding this comment.
networkx has add_node() and add_nodes_from(). Why is the function naming different here?
There was a problem hiding this comment.
I still think the method to add a list of nodes, and a node has to be separate. In addition to the consistency with networkx's API, doing this sort of multiple dispatch is not common in Python, and even with multiple dispatch, List[Node] and Node doesn't have a common superclass that makes sense.
|
What about inheriting directly from |
|
My logic for not doing it that way was that I imagined if sometime in the future they wanted to support different graph packages or provide an option to use them then it would be easier if it was an interface than if it was all just inherited from NetworkX. Since then it could be changed without breaking past uses as the interface would remain the same on the surface. But I have no idea if that was a reasonable thought process. Maybe that would never be an issue? |
|
In that case, for every method that wraps the graph operation, e.g. if graph_is_networkx:
self.G.node[node_id]['agent'].append(agent)
elif graph_is_graphtool:
self.G.vertex(node_id)['agent'].append(agent)
elif graph_is_igraph:
...Each method becomes more costly to run. |
mesa/space.py
Outdated
| if G is not None: | ||
| self.G = G | ||
| elif generator is not None: | ||
| if type(args) is tuple: |
There was a problem hiding this comment.
I think the recommended way for type checks is
isinstance(args, tuple)
You might want to consider changing this here and below.
Alternatively you might just as well skip the check and just use a try...except statement.
This comment applies multiple times below
There was a problem hiding this comment.
I think try/except won't work (least not everywhere) because e.g. anything can be a node in networkx graphs.
But I take on board the isinstance comment, it makes sense that anything inheriting from those types could be valid too.
I can change it once I am sure my toddler is not going to be getting out of bed again!
There was a problem hiding this comment.
Ok I changed that in my branch, is that all I need to do?
There was a problem hiding this comment.
I can't see your changes, maybe you forgot to push your local changes to your forked repo?
There was a problem hiding this comment.
herp derp no, forgot to add changed file!
mesa/space.py
Outdated
|
|
||
| import itertools | ||
|
|
||
| import networkx |
There was a problem hiding this comment.
From what I've seen, the convention tends to be to import networkx as nx -- is there a reason to prefer to keep the full networkx name here?
There was a problem hiding this comment.
Nope no reason, happy to change it.
mesa/space.py
Outdated
|
|
||
| class NetworkGrid: | ||
| """ Network Grid where each node contains zero or more agents. """ | ||
| def __init__(self, G=None, generator="Graph", args=()): |
There was a problem hiding this comment.
Add a docstring explaining the arguments
I think args might work better as a dictionary of named keyword-arguments, i.e.
def __init__(self, G=None, generator="Graph", **kwargs):
There was a problem hiding this comment.
Ok done. Not sure I worded it great though.
tests/test_space.py
Outdated
|
|
||
| import networkx as nx | ||
| import numpy as np | ||
| import pytest |
There was a problem hiding this comment.
The rest of the tests don't use pytest. Is there a reason to prefer it here? Otherwise, it's probably best not to add a new requirement / tool.
There was a problem hiding this comment.
No, I just didn't know about self.assertRaises which I would have if I'd looked through the rest of the tests. Changed.
|
I wrote the review when this was originally submitted, but apparently never actually pressed the button to have the review post -- sorry! re why not just inherit from networkx: I think that networkx is also sufficiently agnostic about what nodes are that having the added functionality that @SophiaWilliams's wrappers add is necessary, even if there's never a different network library underlying it. (Also, if we were to add another library, I'd suggest just subclassing |
|
This has been sitting here (but looking in good shape) for the last six months or so. It looks good to me -- I'm bumping this to allow everyone to take another quick look, and unless anyone spots a new issue I'll approve the PR. Sorry for letting it drag on for so long, @6661696c! |
pre.tests
Outdated
| @@ -0,0 +1,34 @@ | |||
| ============================= test session starts ============================== | |||
There was a problem hiding this comment.
This file is accidentally committed.
| """ Network Grid where each node contains zero or more agents. """ | ||
| """ kwargs = the arguments to the nx generator function | ||
| e.g for watts_strogatz_graph(n, k, p[, seed])""" | ||
| def __init__(self, G=None, generator="Graph", **kwargs): |
There was a problem hiding this comment.
The generator should be an object that defaults to nx.Graph. This is in line with networkx's read_edgelist's create_using argument https://networkx.github.io/documentation/networkx-1.9/reference/generated/networkx.readwrite.edgelist.read_edgelist.html.
5 participants
Never done this before not sure what to write here. Goes with ticket I just opened.