-
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
Change method of partitioning MovieLens #8874
Conversation
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.
Let me know once this is ready for review :)
a15875c
to
ba221c4
Compare
Sure, I am still getting this error and I can't find the reason :| |
This would mean that local neighborhoods are not sorted according to time. |
Yes I know, this error comes from pyg-lib code. But I wonder why it happens, since I used a similar approach as in the temporal_link_pred.py example and no additional sorting was used there so the time data from the dataset should be valid. |
ba221c4
to
c25c10b
Compare
And all local graph store instances respect the constraint from pyg-lib? |
When I check manually, the time values are actually not sorted. The most probable thing to me was that there is an error during partitioning, for example that the data was not permuted correctly. However, looking at the dataset and the corresponding edge_index and time values, the dependencies between them are preserved after the partitioning process. |
@rusty1s I see what's missing. There is an additional sort in here: utils.py#L26. We don't use this function during partitioning. I'll try to fix this as soon as possible. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #8874 +/- ##
==========================================
- Coverage 89.89% 89.25% -0.65%
==========================================
Files 467 467
Lines 29924 29919 -5
==========================================
- Hits 26899 26703 -196
- Misses 3025 3216 +191 ☔ View full report in Codecov by Sentry. |
1df1010
to
dfa38f0
Compare
@rusty1s It’s ready for review. Sorry for the delay. I’m glad you noticed that edge_label_time initialisation. Now I see that the values I provided were so small that it probably discarded the entire neighborhood before checking whether the time data was sorted. I added |
8da97bd
to
e6705bf
Compare
e6705bf
to
34a0aa9
Compare
Changes made: