Adding early stopping to examples and also fix staleness issue with DTDG model embeddings - #397
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
|
||
| test_results = eval(test_loader, test_labels, encoder, decoder, test_metrics) | ||
| log_metrics_dict(test_results, epoch=args.epochs) | ||
| val_score = val_results['BinaryAUROC'] |
There was a problem hiding this comment.
using Binary AUROC here as it is not clear which metric to use @benjaminnNgo
There was a problem hiding this comment.
Note: this fails on integration tests, KeyError for "BinaryAUROC": https://github.com/tgm-team/tgm/actions/runs/22492271186/job/65157420939
ntgbaoo
left a comment
There was a problem hiding this comment.
Fixing stale embeddings looks good to me.
For early stopping, I think we are missing "early stop" as the logic added in this PR just evaluates on the test set for the best epoch and does not really stop when over patience. Not sure if we want to support that.
For ICLR experiments, I have implemented the EarlyStopping class, and I remembered seeing @Jacob-Chmura implement something similar. Do we want to add this logic to the core? Or we can have utils.py within examples and have EarlyStopping class there.
|
|
||
| test_results = eval(test_loader, test_labels, encoder, decoder, test_metrics) | ||
| log_metrics_dict(test_results, epoch=args.epochs) | ||
| val_score = val_results['BinaryAUROC'] |
I think it is a good idea to standardize it. I don't think early stopping should be in core as it doesn't relate to core TG workflow. we can discuss next week. For now, yeah this doesn't implement the full early stopping logic. |
Jacob-Chmura
left a comment
There was a problem hiding this comment.
Running some more integration tests, but approving to unblock.
| z = z.detach() | ||
| h_0 = h_0.detach() | ||
| c_0 = c_0.detach() |
There was a problem hiding this comment.
maybe, but I think it is good practice to detach it to ensure no gradient will go into training the model from eval
| z = encoder(snapshot_batch, static_node_x) | ||
| except StopIteration: | ||
| pass | ||
| z = z.detach() |
Job: test_dygformer_nodeprop_pred_tgbn-trade
Job: test_persistant_forecast_nodeprop_pred_tgbn-trade
Job: test_gcn_nodeprop_pred_tgbn-trade
Job: test_tgcn_nodeprop_pred_tgbn-trade
Job: test_gclstm_nodeprop_pred_tgbn-trade
Job: test_tpnet_nodeprop_pred_tgbn-trade
Job: test_tgat_nodeprop_pred_recency_sampler_tgbn-trade
|
There was a problem hiding this comment.
tgm/examples/nodeproppred/tgn.py
Lines 215 to 230 in 72c8bf9
unrelated to this PR, but I found that we forgot to update edge_feat_dim -> edge_x_dim. Let's fix this while we are updating this file, thanks
Job: test_dygformer_nodeprop_pred_tgbn-trade
Job: test_tgat_nodeprop_pred_recency_sampler_tgbn-trade
Job: test_tgcn_nodeprop_pred_tgbn-trade
Job: test_gcn_nodeprop_pred_tgbn-trade
Job: test_tpnet_nodeprop_pred_tgbn-trade
Job: test_gclstm_nodeprop_pred_tgbn-trade
Job: test_persistant_forecast_nodeprop_pred_tgbn-trade
|
Summary / Description
Added early stopping to all linkproppred, nodeproppred and graphproppred examples
Related Issues: #394
Type of Change
Test Evidence
Requires running integration test to see if anything breaks
Questions / Discussion Points
For graphproppred, it is not clear what metric should use for early stopping @benjaminnNgo