xgi-org / xgi

CompleX Group Interactions (XGI) is a Python package for higher-order networks.
https://xgi.readthedocs.io
Other
171 stars 27 forks source link

Get rid of KeyErrors in to_line_graph #558

Closed pgberlureau closed 3 days ago

pgberlureau commented 5 days ago

Use edge_label_dict.values() as nodes and get edge1 and edge2 from edge_label_dict.keys() to avoid KeyError. (Fix issue #557)

nwlandry commented 4 days ago

@pgberlureau thanks so much for finding this bug and for your contribution! Two quick questions:

  1. Can you add a corresponding unit test in tests/convert/test_line_graph.py called test_557 so that we don't break this in the future?
  2. I realized that we are eliminating duplicate edges by making the tuples of the edges the keys of the label dict. Perhaps we should do something like this:
    for e1, e2 in combinations(H._edge, 2):
    edge_intersect = len(H._edge[e1].intersection(H._edge[e2]))
    if edge_intersect >= s:
        LG.add_edge(e1, e2)
nwlandry commented 4 days ago

I saw that the unit tests are failing due to an unrelated issue which I will fix. Thanks again!!

pgberlureau commented 4 days ago

Hey,

  1. yes i can add the test, no problem :)
  2. You are right, only thing is that in the obtained nx graph, nodes are now indices and the original corresponding hyperedges are attributes of those nodes. This is not a problem to me but is this really the desired behaviour ? (Or I didn't got what you meant sorry)
nwlandry commented 4 days ago

Hey,

  1. yes i can add the test, no problem :)
  2. You are right, only thing is that in the obtained nx graph, nodes are now indices and the original corresponding hyperedges are attributes of those nodes. This is not a problem to me but is this really the desired behaviour ? (Or I didn't got what you meant sorry)

What I was realizing is that using tuples may be unexpectedly buggy. Because the order of set is not guaranteed, when we cast to tuples, we may be getting different permutations of the edges, e.g., (1, 2, 3) and (3, 1, 2). I propose that we fix this using the code that I suggested, or, at the very least, putting a sorted statement inside the casting to tuple.

My worry about using the edges as keys is that the line graph will be the same with or without multiedges, so if a hypergraph is not a simple hypergraph, then we're not getting the accurate linegraph.

pgberlureau commented 4 days ago

Yes, indeed tuple(set([-1,-2])) == tuple(set([-2,-1])) is False.

I just suggest we use your code but we keep original hyperedges as data of the corresponding created nodes in the linegraph so we don't lose information (which can be useful with spatial hypergraph for instance).

nwlandry commented 4 days ago

Yes, indeed tuple(set([-1,-2])) == tuple(set([-2,-1])) is False.

I just suggest we use your code but we keep original hyperedges as data of the corresponding created nodes in the linegraph so we don't lose information (which can be useful with spatial hypergraph for instance).

That sounds perfect!

pgberlureau commented 3 days ago

I have nothing to add, thank you for your work !