search

StatsDLMathsRecomSys / Inductive-representation-learning-on-temporal-graphs

286 stars 61 forks source link

Why is nn_val_rand_sampler unused? #5

Open rocketrose99 opened 3 years ago

rocketrose99 commented 3 years ago

Hey guys!

I noticed that in learn_edge.py, nn_val_rand_sampler is defined on line 209,

nn_val_rand_sampler = RandEdgeSampler(nn_val_src_l, nn_val_dst_l)

but never used. Should it be used instead of val_rand_sampler on line 282?

nn_val_acc, nn_val_ap, nn_val_f1, nn_val_auc = eval_one_epoch('val for new nodes', tgan, val_rand_sampler, nn_val_src_l, nn_val_dst_l, nn_val_ts_l, nn_val_label_l)

Other than that, I got TGAT running on my dataset and it's lookin good!

justinyeh1995 commented 3 years ago

I believe so

richardruancw commented 2 years ago

Thanks for flagging this.

Yes, the line:https://github.com/StatsDLMathsRecomSys/Inductive-representation-learning-on-temporal-graphs/blob/66b2ccdd6d466f342900d8837c21224a49eda7e5/learn_edge.py#L282

should use nn_val_rand_sampler instead of val_rand_sampler. This does not impact training progress as the early stop is determined by val_ap which is from the old nodes. But the logging of the evaluation metrics for new nodes on validation data is incorrect due to this bug.

I will send a PR to fix this.