KGCN-pytorch icon indicating copy to clipboard operation
KGCN-pytorch copied to clipboard

Make several improvements

Open Ki-Seki opened this issue 1 year ago • 2 comments

I have made several improvements and it would be an honor if you could review my code. Please feel free to decide whether to accept this pull request or not.

Comparison with PyTorch KGCN

Add a new handcrafted dataset product

  • Dataset source: Rec-Tmall
  • Preprocessing script: preprocessing.ipynb

Add a new mixer transe

transe mixer has a better performace on divergence speed, time complexity, divergence loss value and AUC value, final-epoch loss value and AUC value in most cases.

See _mix_neighbor_vectors_TransE() in aggregator.py

Add batch_experiments.ipynb for convenient experiment

Automatically conduct multiple experiments.

  1. Config many parameter sets once
  2. Run
  3. Check results in a well-formatted print-out

Fix RuntimeError: indices should be either on cpu or on the same device as the indexed tensor (CPU)

Before

https://github.com/zzaebok/KGCN-pytorch/blob/3b0bb56da4b6759d204de06f1d4547e9b4abe3ce/model.py#L79-L80

After

            neighbor_entities = torch.LongTensor(self.adj_ent[entities[h].cpu()]).view((self.batch_size, -1)).to(self.device)
            neighbor_relations = torch.LongTensor(self.adj_rel[entities[h].cpu()]).view((self.batch_size, -1)).to(self.device)

Fix IndexError: index out of range in self

Before

https://github.com/zzaebok/KGCN-pytorch/blob/3b0bb56da4b6759d204de06f1d4547e9b4abe3ce/model.py#L34-L35

After

        self.adj_ent = torch.zeros(self.num_ent, self.n_neighbor, dtype=torch.long)
        self.adj_rel = torch.zeros(self.num_ent, self.n_neighbor, dtype=torch.long)

Improve readability

  • Add more comments
  • Reorganize some codes
  • etc.

Ki-Seki avatar Apr 02 '23 16:04 Ki-Seki

Hi,

Thank you for the valuable PR!

Before I review the code, let me emphasize what I consider important: the relevance of the original code(https://github.com/hwwang55/KGCN). This repository is essentially a PyTorch implementation of KGCN. Therefore, it should be helpful in understanding how KGCN works. Consequently, I prefer simplicity and understandability over the performance of the implementation. In this context, I really appreciate that you created a file like KGCN.py to easily run the experiments.

Before I look into your code, I have some questions. Please take a look and let me know the answers.

  1. You attached .cpu() to the tensors to fix the RuntimeError: indices should be either on cpu or on the same device as the indexed tensor (CPU). As far as I know, this feature moves tensors from GPU to CPU. I don't know which devices you used for training/evaluation, but using cpu() could be a performance issue for those who use GPU. Could you please provide more details?

  2. You changed torch.empty to torch.zeros to fix IndexError: index out of range in self. However, I cannot find information indicating that only torch.empty can generate an index error. Can you please provide a reference?

zzaebok avatar Apr 04 '23 05:04 zzaebok

For the first one, I do use cuda. I move the tensor to cpu because the error information tells me to do so. I also don't know why it says the indices and indexed tensors are on different devices. For the second one, I suppose using torch.zeros will be more robust because the 0-index will never cause an out-of-range error. Also, I don't know why torch.empty doesn't work. Though it should work because we know the values inside the tensor will be replaced with valid indices. I'm so sorry that my justifications are not detailed enough for I've just started learning those things.

Ki-Seki avatar Apr 13 '23 07:04 Ki-Seki