Adding TGCN for nodeproppred - #89
Conversation
Added initial development of TGCN and updated __init__.py with new model class
Fixed importing wrong lib for zeros() init. Added NodePropPred example for TGCN. Added Integration test for TGCN
|
Not sure why the test check on GitHub failed. The test ran successfully on my local (except 3 tests failed due to the issue mentioned in this PR #82) |
Safe to ignore these tests. There was an issue in the test cases due to auto-merging conflicts in some PRs that I pushed. I have adressed these and the fix should be in master shortly. |
Jacob-Chmura
left a comment
There was a problem hiding this comment.
Very nice work. Just some minor comments. I am not familiar with the TGCN paper, @shenyangHuang please review the core library layer and make a judgement on the expected performance.
| '--gres=gpu:a100l:1', | ||
| ] | ||
| ) | ||
| def test_gclstm_nodeprop_pred(slurm_job_runner, dataset): |
There was a problem hiding this comment.
| def test_gclstm_nodeprop_pred(slurm_job_runner, dataset): | |
| def test_tgcn_nodeprop_pred(slurm_job_runner, dataset): |
| r"""python -u gclstm.py --dataset tgbn-trade --time-gran r --batch-time-gran r | ||
| python -u gclstm.py --dataset tgbn-genre --time-gran s --batch-time-gran D\ | ||
| example commands to run this script. | ||
| """ |
There was a problem hiding this comment.
| r"""python -u gclstm.py --dataset tgbn-trade --time-gran r --batch-time-gran r | |
| python -u gclstm.py --dataset tgbn-genre --time-gran s --batch-time-gran D\ | |
| example commands to run this script. | |
| """ | |
| """ |
I think this can be removed
| edge_index = torch.stack([batch.src, batch.dst], dim=0) | ||
| edge_weight = batch.edge_weight if hasattr(batch, 'edge_weight') else None # type: ignore | ||
| z, h_0 = self.encoder(node_feat, edge_index, edge_weight, h_0) | ||
| z_node = z[batch.nid_to_idx[batch.node_ids]] # type: ignore |
There was a problem hiding this comment.
After merging #60, the batch attribute to use is actually global_to_local.
| z_node = z[batch.nid_to_idx[batch.node_ids]] # type: ignore | |
| z_node = z[batch.global_to_local[batch.node_ids]] # type: ignore |
| self, | ||
| x: torch.Tensor, | ||
| edge_index: torch.Tensor, | ||
| edge_weight: torch.Tensor, |
There was a problem hiding this comment.
| edge_weight: torch.Tensor, | |
| edge_weight: torch.Tensor | None, |
| parser.add_argument('--seed', type=int, default=1337, help='random seed to use') | ||
| parser.add_argument('--dataset', type=str, default='tgbn-genre', help='Dataset name') | ||
| parser.add_argument('--device', type=str, default='cpu', help='torch device') | ||
| parser.add_argument('--epochs', type=int, default=100, help='number of epochs') |
There was a problem hiding this comment.
Just double checking if we want to use 100 or 10 as default number of epochs
There was a problem hiding this comment.
for DTDG methods usually takes more epochs
| edge_index (PyTorch Long Tensor): Graph edge indices. | ||
| edge_weight (PyTorch Long Tensor, optional): Edge weight vector. | ||
| H (PyTorch Tensor, optional): Hidden state matrix for all nodes. | ||
| C (PyTorch Tensor, optional): Cell state matrix for all nodes. |
There was a problem hiding this comment.
| C (PyTorch Tensor, optional): Cell state matrix for all nodes. |
There was a problem hiding this comment.
Unless we need this tensor in which case example should be updated
Codecov ReportAttention: Patch coverage is
📢 Thoughts on this report? Let us know! |
|
test performance of 0.3654 is quite sensible. |
| in_channels: int, | ||
| out_channels: int, | ||
| improved: bool = False, | ||
| cached: bool = False, |
There was a problem hiding this comment.
when would you want to turn this one? what would be the benefit?
There was a problem hiding this comment.
improved and cached are both used to configure GCNConv. By default, they are set to False. From what I am understanding from reading the documentation from PyG.
- If we set
improved = True, the self-loops are addedA+2Iinstead ofA+I. This increases the weight of self-loops, giving each node’s own features more influence during aggregation. - If we set
cached = True, the layer computes the normalized adjacency matrix only once, and reuses it. This speeds up training and inference but is only suitable for transductive learning. Scenarios wherecachedcan be set toTrueinclude tasks such as traffic forecasting, where graph structure is assumed to be static and only node features are dynamic.cachedshould not be set toFalsewhen graph structure dynamically changes.
Thanks for pointing this out. These are all useful information that we should include in the comment section.
There was a problem hiding this comment.
yeah thanks for adding the comments here, much more clear now
| parser.add_argument('--seed', type=int, default=1337, help='random seed to use') | ||
| parser.add_argument('--dataset', type=str, default='tgbn-genre', help='Dataset name') | ||
| parser.add_argument('--device', type=str, default='cpu', help='torch device') | ||
| parser.add_argument('--epochs', type=int, default=100, help='number of epochs') |
There was a problem hiding this comment.
for DTDG methods usually takes more epochs
Increase default number of epoch to 250
|
I added further information about |
| in_channels: int, | ||
| out_channels: int, | ||
| improved: bool = False, | ||
| cached: bool = False, |
There was a problem hiding this comment.
yeah thanks for adding the comments here, much more clear now
|
The new performance makes sense to me, I got similar range from GCN yesterday for nodeproppred. |

Objective
Adding the implementation of T-GCN and a node property prediction example for T-GCN
New scripts
Testing evidence
Command used:
tgbn-trade(GCLSTMachieved 0.2462 NDCG)TODO for another PR: