Adding rich-core function and test #2706
Conversation
adding rich-core function
adding rich-core test function
|
Looks like you need to add an import of richcore into the |
networkx/algorithms/rich_core.py
Outdated
|
|
||
|
|
||
| import networkx as nx | ||
| from networkx.utils import accumulate |
There was a problem hiding this comment.
I don't think this import is needed.
networkx/algorithms/rich_core.py
Outdated
| def extract_rich_core(G, weight='weight'): | ||
| """Returns the core/periphery structure of a weighted undirected network (see [1]). | ||
|
|
||
| Args |
There was a problem hiding this comment.
This section of the docstring should be called Parameters.
Look at other modules to see the expected format.
Also, it would be good to summarize the definition of core/periphery structure in a short paragraph separated from the one-line description by a blank line.
It looks like your docstring is indented an extra space. Some lines may also be too long. Check with the pep8 tool (which checks if your code follows PEP8 standards for python code).
You should use :math: mode for the equations in the docstring.
networkx/algorithms/rich_core.py
Outdated
| G = G.to_undirected() | ||
|
|
||
| #Looking for the minimal weight | ||
| weights = [e[2][weight] for e in G.edges_iter(data=True)] |
There was a problem hiding this comment.
G.edges_iter won't work with NetworkX V2.0.
Also, you can avoid creating a list of all weights.
Use: minw = min(wt for u, v, wt in G.edges.data(weight))
networkx/algorithms/rich_core.py
Outdated
| minw = min(weights) | ||
|
|
||
| #Normalising by the minimal weight | ||
| for e in G.edges_iter(data=True): |
There was a problem hiding this comment.
Perhaps: for u, v, wt in G.edges.data(weight):
Also, you are mutating the edge weight attribute in this function. A warning should be added to the doc_string or avoided by creating another attribute to store those values. Or even better -- just divide the sigma_i by minw when you store it. Then the original graph doesn't change and I think your calculations and sorting is the same. You also avoide this loop that way.
| sigmas.append(sigma_i) | ||
|
|
||
| r_star = sigmas.index(max(sigmas)) | ||
|
|
There was a problem hiding this comment.
You should consider a different return data structure.
Of course, it may depend what you expect people to do with the results.
A dict from nodes to sigma values is our usual way to return node properties.
This loses the order of rankings as well as r_star.
Perhaps r_star could be replaced by the node with maximal sigma -- since at this point it is a index of that node. Or you could just give the user the dict of sigma values and let the user find the node with max sigma value.
| G.add_edge('a','d',weight=0.3) | ||
|
|
||
| sigmas, ranked_nodes, r_star = nx.richcore.extract_rich_core(G, weight='weight') | ||
| assert_equal(sigmas, [0, 19.0, 19.0, 19.0, 10.999999999999998, 30.0]) |
There was a problem hiding this comment.
tests with floating point numbers are tricky to get exactly equal.
You might consider from nose.utils import assert_almost_equal for the tests.
There is also a nice way to make sure division works for floating point for python2 and 3. Use this at the top of the rich_core.py module: from __future__ import division Then you don't need to use 1.* when you divide.
|
I think the sigma values you return are actually the sigma^+ values, not the sigmas. It also looks like the normalization of weights is not needed (or can be done after all computations). |
| "Rich-cores in networks". | ||
| PLoS One 10(3):e0119678. | ||
| """ | ||
| if nx.is_directed(G): |
There was a problem hiding this comment.
This is not needed, since there is decorator that checks it.
|
|
||
| #Normalising by the minimal weight (adding a new attribute) | ||
| for u, v, wt in G.edges.data(weight): | ||
| G[u][v]['norm_weight']=wt/minw |
There was a problem hiding this comment.
Let's add a note in the documentation that this function modifies the graph.
| import networkx as nx | ||
| from collections import OrderedDict | ||
| from networkx.utils import not_implemented_for | ||
| from __future__ import division |
There was a problem hiding this comment.
Must be before any other imports.
|
|
||
| .. math:: | ||
|
|
||
| \sigma_i = \sum_j \frac{w_ij}{w_{min}} |
There was a problem hiding this comment.
I think it should be w_{ij}.
| @not_implemented_for('directed') | ||
| @not_implemented_for('multigraph') | ||
| def extract_rich_core(G, weight='weight'): | ||
| r"""Returns the core/periphery structure of a the weighted undirected |
There was a problem hiding this comment.
The first sentence should fit in one line.
| @not_implemented_for('directed') | ||
| @not_implemented_for('multigraph') | ||
| def extract_rich_core(G, weight='weight'): | ||
| r"""Returns the core/periphery structure of a the weighted undirected |
| for j in G.neighbors(i): | ||
| if norm_strength[j]>norm_strength[i]: | ||
| sigma_i += norm_strength[j] | ||
| else: continue |
There was a problem hiding this comment.
This line is unnecessary.
| norm_strength = G.degree(weight='norm_weight') | ||
| ranked_nodes = [n[0] for n in sorted(norm_strength, key=lambda x: x[1], reverse=True)] | ||
|
|
||
| sigmas=OrderedDict() |
There was a problem hiding this comment.
In this line and others there is missing whitespace around the operator.
|
Needs tests and updates to reflect reviews above. |
|
Thanks for this @iaciac! This PR hasn't been touched in a while and will need some effort to make it work with the |
Extraction of the rich-core of complex networks ad in Ma A and Mondragón RJ (PLoS One, 2015).