-
Notifications
You must be signed in to change notification settings - Fork 30
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
Line graph and vector centrality functions added #290
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dear Gonzalo, thank you for your contribution! The entire purpose of XGI from its inception was to be a tool for the community by the community, so your involvement is more than welcome. I have reviewed the code and it looks like exactly the kind of addition we are always open to receiving. I have left some comments that can hopefully be solved in a few quick iterations and we'll be sure to merge this after that.
Perhaps another pair of eyes would be useful too @nwlandry @maximelucas.
After all the comments are resolved, it would also be nice to have one or two reference tests for the new features.
Thanks again!
|
||
D = np.max(list(hyperedge_dims.values())) | ||
|
||
for k in range(2, D + 1): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This loop is first iterating over each possible edge order, then filtering to get all edges of said order, and the iterating over each node of each edge of those edges. So it's actually a four-ly nested loop. I wonder if there is a way to iterate over each edge (and each node in each edge) once?
c_i[node] += LGcent[edge_label_dict[edge]] | ||
except IndexError: | ||
raise Exception( | ||
"Nodes must be written with the Pythonic indexing (0,1,2...)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not super clear to me what is mean there by Pythonic indexing. Does this mean that the algorithm does not work for nodes that are labeled with strings? Does it work with nodes that are labeled with non-consecutive integers, or integers not starting at zero?
Co-authored-by: Leo Torres <dleonardotn@gmail.com>
Changed vector_centrality to line_vector_centrality
Co-authored-by: Leo Torres <dleonardotn@gmail.com>
Thanks a lot @goznalo-git ! This looks like a quality PR. |
xgi/convert.py
Outdated
edge_label_dict = { | ||
tuple(edge): index for index, edge in enumerate(H.edges.members()) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Current code will give new labels to the hyperedges from 0 to M. This suggestion keeps the hyperedges labels, which might be unordered, and strings etc. Check it does not mess up the rest of your code.
edge_label_dict = { | |
tuple(edge): index for index, edge in enumerate(H.edges.members()) | |
} | |
edge_label_dict = {tuple(edge): index for index, edge in H._edge} |
for edge1, edge2 in combinations(H.edges.members(), 2): | ||
if edge1.intersection(edge2): | ||
LG.add_edge(edge_label_dict[tuple(edge1)], edge_label_dict[tuple(edge2)]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your code definitely works. I'm wondering if it might be faster using H.edges.neighbors()
. I give it a try below, feel free to discard.
for edge1, edge2 in combinations(H.edges.members(), 2): | |
if edge1.intersection(edge2): | |
LG.add_edge(edge_label_dict[tuple(edge1)], edge_label_dict[tuple(edge2)]) | |
edges = [] | |
for edge_id1 in H.edges(): | |
for edge_id2 in H.edges.neighbors(edge_id1): | |
edges.append({edge_id1, edge_id2}) | |
LG.add_edges_from(edges) |
xgi/algorithms/centrality.py
Outdated
edge_label_dict = { | ||
tuple(edge): index for index, edge in enumerate(H.edges.members()) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you need labeling from 0 to M? If not see below for suggestion to keep hyperedges labels
|
||
D = H.edges.size.max() | ||
|
||
for k in range(2, D + 1): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this go up to D
since you're iterating over sizes?
for k in range(2, D + 1): | ||
c_i = np.zeros(len(H.nodes)) | ||
|
||
for edge, _ in list(filter(lambda x: x[1] == k, hyperedge_dims.items())): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should also work with (check if not slower). Then you don't need to create hyperedge_dims
anymore
for edge, _ in list(filter(lambda x: x[1] == k, hyperedge_dims.items())): | |
for edge in H.edges.filterby("size", k).members(): |
Thank you very much @goznalo-git. I added some suggestions to make use of some xgi features. |
Two suggestions added, the rest have been replied to. Co-authored-by: Maxime Lucas <maximelucas@users.noreply.github.com>
Thanks for your feedback! There were some interesting suggestions and improvements, indeed. Let me know what you think of the actual status. best! |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #290 +/- ##
=======================================
Coverage ? 83.41%
=======================================
Files ? 35
Lines ? 2732
Branches ? 0
=======================================
Hits ? 2279
Misses ? 453
Partials ? 0 Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
Thank you @goznalo-git. I'll approve this for merging now, and raise some issues for our remaining comments so we can solve later together with adding tests. |
@goznalo-git, thank you for your contribution. You are more than welcome to continue contributing anything you'd find useful, and to come chat with us whenever on our zulip server. |
Hi there,
I really like this library, and I've been using it for a while now. I would like to have these two additions included, and I'm sure others would benefit from them.
convert_to_line_graph()
takes a Hypergraph and constructs its associated line graph (a pairwise graph whose nodes correspond to the edges of the Hypergraph, and two of those nodes are linked together if their associated edges share at least one node).vector_centrality()
, defined in https://doi.org/10.1016/j.chaos.2022.112397, which allows one to extract a vector-like eigenvector centrality of a Hypergraph by using the abovementioned connection to its line graph.I updated the changelog and the docs, as well as making sure the formatting was fine with isort and black.
best,
Gonzalo