pyt-team / challenge-icml-2024

Official repository for the Topological Deep Learning Challenge 2024, organized by TAG-DS & PyT-Team and hosted by GRaM Workshop @ ICML 2024.
https://pyt-team.github.io/packs/challenge.html
MIT License
37 stars 48 forks source link

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

Open mbanf opened 2 months ago

mbanf commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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.