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

bugfix in Modules/utils/utils.py in the plot_manual_graph function #36

Open
mbanf opened this issue Jul 7, 2024 · 2 comments
Open

bugfix in Modules/utils/utils.py in the plot_manual_graph function #36

mbanf opened this issue Jul 7, 2024 · 2 comments
Assignees

Comments

@mbanf
Copy link

mbanf commented Jul 7, 2024

bugfix in Modules/utils/utils.py in the plot_manual_graph function, switched indexing for nodes instead of hyperedges in incidence matrix results in too many added nodes and hence an out of index error. This error does not happen in the KNN given example, because the
number of hyperedges equals the number of nodes and hence doubles the number of nodes anyways. It occurs however, if the number of hyperedges is less then the number of nodes”

image
@Coerulatus
Copy link
Contributor

Hello @mbanf,
thank you for bringing forward this issue. Could you give me an example in which the error arises? I tried removing and adding a hyperedge in the knn_lifting tutorial but I don't see any error. It is also worth noting that while there could be an error, your solution plots the incorrect bipartite graph.
This is the code I used to test the function with a different number of nodes (it can be pasted at the end of the knn_lifting tutorial notebook):

import torch
from modules.utils.utils import plot_manual_graph

data = lifted_dataset.get(0)
data.incidence_hyperedges = torch.sparse_coo_tensor(indices=[[0,0,0,1,1,1,2,2,2,3,3,3,4,4,4,5,5,5,6,6,6,6],[0,1,2,0,1,2,0,1,2,1,2,3,2,3,4,3,4,5,4,5,6,7]], values=[1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1], size=(8,7))
data.num_hyperedges = 7
print(data)

plot_manual_graph(data)

data = lifted_dataset.get(0)
data.incidence_hyperedges = torch.sparse_coo_tensor(indices=[[0,0,0,1,1,1,2,2,2,3,3,3,4,4,4,5,5,5,6,6,6,7,7,7,8,8,8],[0,1,2,0,1,2,0,1,2,1,2,3,2,3,4,3,4,5,4,5,6,5,6,7,5,6,7]], values=[1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1], size=(8,9))
data.num_hyperedges = 9
print(data)

plot_manual_graph(data)

@PierrickLeroy
Copy link

Hello @Coerulatus @mbanf,
I am working on the challenge also on a graph to hypergraph scenario and while I might have missed something, I made the same analysis. I think the indices should be switched. It is not seen as a bug on knn lifting because there is as many edges as nodes and if I remember correctly there is a bug in knn lifting also which makes the incidence hyperedge matrix the transpose of what it should be. If it is indeed correct to think that nodes should be rows and hyperedges columns then from my point of view the change makes sense.
These two inversions ie 1) in the plot function and 2) in the knn lifting code makes it confusing.

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

No branches or pull requests

3 participants