Update clustering.py #608
Update clustering.py #608
Conversation
Fixing local_clustering_coefficient function
|
Looks good @cosimoagostinelli! Thanks for the contribution! |
|
The only thing is that can you make sure that the unit tests pass? It looks like they need to be updated. |
|
The tests failing seem to be in |
|
I think that the output given by my version of the function is the correct one, according to the definition proposed in "Properties of metabolic graphs: biological organization or representation artifacts?" (ref. 1 of the local_clustering function) while the test does not match this defeinition. Indeed, in the test we have 3 nodes and 4 hyperedges: e1=[0,1], e2=[0,2], e3=[1,2], e4=[0,1,2]. For instance, for the node 0 we have: EO(e1,e2) = 1 -- because nodes 1 and 2 are connected through at least another edge. Thus, the clustering is given by (1+0+0)/3. This is true for all the nodes as they are equivalent. |
|
Does it make sense? |
|
Hi @cosimoagostinelli --- absolutely! I agree with you. What I meant was not that your new version of the function is wrong, but rather, could you update the unit tests so that they are also correct and match your new function? Sorry about the confusion. |
|
Oh, sorry, I completely misunderstood! |
Update test 2 and 6 for local_clutering_coefficient and added a 7th test with respect to pairwise clustering coefficient (networkx).
| 3: 0.625, | ||
| 4: 0.5833333333333334, | ||
| 5: 1.0, | ||
| 6: 1.0 |
There was a problem hiding this comment.
nice this make more sense, since 6's two neighbours are connected
| } | ||
| for n in cc: | ||
| assert round(cc[n], 3) == round(true_cc[n], 3) | ||
|
|
||
| G = nx.erdos_renyi_graph(50, 0.1, seed=0) |
There was a problem hiding this comment.
| G = nx.erdos_renyi_graph(50, 0.1, seed=0) | |
| # fix #607 to ensure it matches standard cc in pairwise case | |
| G = nx.erdos_renyi_graph(50, 0.1, seed=0) |
just for future reference
|
Mini comment but looks good to me, thanks Cosimo! I'll let Nich approve since I think he wrote this function. Edit: there's still examples in the docs that are failing the tests. Can you update the example there too? |
update docs
update docs local_clustering_coefficient
|
Great work @cosimoagostinelli !! Merging now. |
|
Thank you, happy to contribute! |
|
Congrats on your first PR!! |
Fixing local_clustering_coefficient function.
I think that the two 'for' cycles are not correct because 'range(dv)' does not identify the hyper-edges to which the node n belongs.
Since we have to go over all the possible pairs of hyperedges (without repetitions) containing the node n, I guess that one possible correct implementation may be: 'for e1, e2 in combinations(ev, 2)'.