-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Fix LinkNeighborLoader
producing double-sized edge_label_time
for homogeneous graphs
#7807
Conversation
60ea03b
to
00b542a
Compare
for more information, see https://pre-commit.ci
This reverts commit a3e6801.
f162dea
to
51f30c9
Compare
841e7ec
to
2c95b33
Compare
LinkNeighborLoader
LinkNeighborLoader
producing double-sized edge_label_time
for homogeneous graphs
|
||
for sample in loader: | ||
assert sample.edge_label_index.size() == (2, batch_size) | ||
assert sample.edge_label_time.size() == (batch_size, ) |
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.
Before this PR, the size of edge_label_time
was (2*batch_size, )
.
Codecov Report
@@ Coverage Diff @@
## master #7807 +/- ##
=======================================
Coverage 91.57% 91.58%
=======================================
Files 455 455
Lines 25670 25656 -14
=======================================
- Hits 23507 23496 -11
+ Misses 2163 2160 -3
... and 5 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
# TODO(akihironitta): Investigate the failure | ||
# assert torch.all(sample.edge_time <= sample.edge_label_time) |
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.
Could someone also confirm this assertion is correct before I dig into this?
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.
Yes, will do.
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. I fixed the assert that was commented out. The issue was that time
needs to be a node-level vector (for now), and that the endpoints of an edge needs to exist in time.
Fixes
edge_label_time.size() == (2*batch_size,)
to have(batch_size,)
.Adds a test case for #7791.
Part of #7796 and #6528.