-
Notifications
You must be signed in to change notification settings - Fork 3.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add LinkEncoder
of GraphMixer
#7459
Add LinkEncoder
of GraphMixer
#7459
Conversation
05280a8
to
3fb13a2
Compare
Codecov Report
@@ Coverage Diff @@
## master #7459 +/- ##
==========================================
- Coverage 91.89% 91.59% -0.31%
==========================================
Files 452 452
Lines 25292 25360 +68
==========================================
- Hits 23243 23229 -14
- Misses 2049 2131 +82
... and 17 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Assuming each input node has the same number of temporal edges. If the number varies, the input needs zero-padding in advance to forward pass.
22e05e2
to
8006e2b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this! Quick initial comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this! Quick initial comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the hard work. My final round of comments to address.
torch_geometric/nn/encoding.py
Outdated
edge_attr_time, _ = to_dense_batch( | ||
edge_attr_time, | ||
edge_index[1], | ||
max_num_nodes=self.K, | ||
batch_size=num_nodes, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good point. If a destination node has degree larger than K
, we will need to include K
mst recent edges, i.e., largest K
elements based on edge_time
for the destination node.
I think this looks all good. Let's try to get this in. My only concern is that we should move this functionality to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@akihironitta Thanks for the great work.
@weihua916 I updated the PR, the latest K edge attributes are used to create the node embedding now. Appreciate review and approval from you before we merge.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks for the great work.
Co-authored-by: Jintang Li <cnljt@outlook.com>
Co-authored-by: Jintang Li <cnljt@outlook.com>
This PR introduces the "link-encoder" defined in 3.1 of the GraphMixer paper, Do We Really Need Complicated Model Architectures For Temporal Networks?.