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

Get rid of KeyErrors in to_line_graph #558

Merged
merged 4 commits into from
Jul 4, 2024

Conversation

pgberlureau
Copy link
Contributor

@pgberlureau pgberlureau commented Jul 3, 2024

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

@thomasrobiglio thomasrobiglio linked an issue Jul 3, 2024 that may be closed by this pull request
xgi/convert/line_graph.py Outdated Show resolved Hide resolved
@nwlandry
Copy link
Collaborator

nwlandry commented Jul 3, 2024

@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
Copy link
Collaborator

nwlandry commented Jul 3, 2024

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

@pgberlureau
Copy link
Contributor Author

pgberlureau commented Jul 3, 2024

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
Copy link
Collaborator

nwlandry commented Jul 3, 2024

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
Copy link
Contributor Author

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
Copy link
Collaborator

nwlandry commented Jul 3, 2024

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!

Copy link
Collaborator

@nwlandry nwlandry left a comment

Choose a reason for hiding this comment

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

This looks awesome, @pgberlureau! Thanks so much! Let me know if you're done, and if so, I'll merge.

@pgberlureau
Copy link
Contributor Author

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

@nwlandry nwlandry merged commit bc90a38 into xgi-org:main Jul 4, 2024
22 checks passed
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.

to_line_graph function leads to KeyError
3 participants