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

Added solutions for some basic tasks #5

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

VRajesh7649
Copy link

Added solutions for some of the basic benchmark tasks.

I hope I did this right; please let me know if any adjustments need to be made.

This is for igraph/python-igraph#469.

@VRajesh7649 VRajesh7649 marked this pull request as draft December 13, 2022 04:06
@szhorvat szhorvat requested a review from ntamas December 13, 2022 07:11
Copy link
Member

@ntamas ntamas left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution! I've added a few suggestions with possible improvements.

for e in g.es:
union(e.source, e.target)
connected_components = dict()
for i in range(20):
Copy link
Member

Choose a reason for hiding this comment

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

Do not hardcode the number of vertices here.


for e in g.es:
union(e.source, e.target)
connected_components = dict()
Copy link
Member

Choose a reason for hiding this comment

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

You can use a defaultdict from the collections module to avoid having to deal with the case when you are adding a vertex to the component for the first time.

g = ig.Graph.Erdos_Renyi(20, m=8, directed=False, loops=False)
wcc = weakly_connected_components(g)
colors = ["aliceblue", "antiquewhite", "aqua", "aquamarine", "beige", "navyblue", "orchid", "blue", "blueviolet", "brown", "cadetblue", "chartreuse", "cornflowerblue", "gold", "darkolivegreen", "darksalmon", "firebrick", "goldenrod", "khaki"]
for a in range(len(wcc.keys())):
Copy link
Member

Choose a reason for hiding this comment

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

You can simply iterate over the key-value pairs of the dictionary with for key, value in wcc.items()

for a in range(len(wcc.keys())):
i = list(wcc.keys())[a]
for j in wcc[i]:
g.vs[j]["color"] = colors[a]
Copy link
Member

Choose a reason for hiding this comment

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

This can be done in a single call: g.vs[wcc[i]]["color"] = colors[a] * len(wcc[i]). Also note that wcc[i] is unnecessasry if you use something like for members in wcc.values() for the iterations (it seems like you won't need the key itself).

kneighborhood = []
distances = g.distances(v, g.vs, weights=None, mode='all')[0]
for i in range(n):
if distances[i] == k:
Copy link
Member

Choose a reason for hiding this comment

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

I think this should be <= k here, not == k, but maybe @szhorvat meant something different when he originally specified the benchmark.

Nevertheless, you can simplify this greatly with a Python list comprehension. Also note that you need to exclude v itself:

kneighborhood = [i for i, d in enumerate(distances) if d <= k and i != v.index]

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I meant <= k.

Copy link
Member

Choose a reason for hiding this comment

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

Regarding distances = g.distances(v, g.vs, weights=None, mode='all')[0],

  • target should be left at None. @ntamas In the C layer, the igraph_vss_all() path is better optimized for many functions, and will perform better than listing all vertices explicitly. I assume None maps to igraph_vss_all(). What about g.vs? Does it map to vss_all() or to something for which igraph_vs_is_all() is not true?
  • Generally, computing the distance to all other vertices, even those further than k steps away, will be quite inefficient. Using the neighborhood() method will be better here.

if distances[i] == k:
kneighborhood.append(i)
for i in kneighborhood:
res[v.index] += g.degree(i)
Copy link
Member

Choose a reason for hiding this comment

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

You can query g.degree() in advance outside the for loop and just index into it. This avoids querying the degree of the same vertex multiple times if the vertex occurs in the neighborhood of many other vertices.

for i in kneighborhood:
res[v.index] += g.degree(i)
res[v.index] /= len(kneighborhood)
res[v.index] = round(res[v.index], 2)
Copy link
Member

Choose a reason for hiding this comment

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

Rounding was not asked for in the original task.

@@ -0,0 +1,13 @@
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.

Please specify which benchmark task this file is solving.

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