Skip to content
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

Require sorted neighborhoods according to time in temporal sampling #108

Merged
merged 5 commits into from
Sep 22, 2022

Conversation

ZenoTan
Copy link
Member

@ZenoTan ZenoTan commented Sep 21, 2022

As discussed offline, we need to unify the implementation for temporal and non-temporal sampling.
This PR updates the neighbor sampling kernel by deciding the range of sampling based on seed node time. We assume we have sorted each neighborhood by time. This assumption will be guaranteed on pyg side in the follow-up PRs.

@codecov-commenter
Copy link

codecov-commenter commented Sep 21, 2022

Codecov Report

Attention: Patch coverage is 85.71429% with 4 lines in your changes missing coverage. Please review.

Project coverage is 84.70%. Comparing base (34c08c9) to head (79c7aae).
Report is 180 commits behind head on master.

Files Patch % Lines
pyg_lib/csrc/sampler/cpu/neighbor_kernel.cpp 85.71% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #108      +/-   ##
==========================================
+ Coverage   78.50%   84.70%   +6.20%     
==========================================
  Files          16       16              
  Lines         428      412      -16     
==========================================
+ Hits          336      349      +13     
+ Misses         92       63      -29     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@rusty1s rusty1s changed the title Refactor temporal sampling (Part 1) - Require sorted neighborhoods according to time in temporal sampling Sep 22, 2022
@rusty1s rusty1s changed the title - Require sorted neighborhoods according to time in temporal sampling Require sorted neighborhoods according to time in temporal sampling Sep 22, 2022
@rusty1s
Copy link
Member

rusty1s commented Sep 22, 2022

This is amazing, thank you! Also added a note to the documentation.

@rusty1s rusty1s enabled auto-merge (squash) September 22, 2022 07:13
@rusty1s rusty1s merged commit c7d12f2 into master Sep 22, 2022
@rusty1s rusty1s deleted the zy_time branch September 22, 2022 07:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants