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.

Sign up for GitHub

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

Jump to bottom

NeighborSampler: Sort local neighborhoods according to time #5516

Merged
merged 9 commits into from
Sep 29, 2022
Merged

NeighborSampler: Sort local neighborhoods according to time #5516

merged 9 commits into from
Sep 29, 2022

Conversation

rusty1s
Copy link
Member

@rusty1s rusty1s commented Sep 23, 2022

No description provided.

@rusty1s rusty1s self-assigned this Sep 23, 2022
@codecov
Copy link

codecov bot commented Sep 23, 2022 •
edited
Loading

Codecov Report

Merging #5516 (6315f33) into master (9b4b854) will increase coverage by 0.00%.
The diff coverage is 92.00%.

@@           Coverage Diff           @@
##           master    #5516   +/-   ##
=======================================
  Coverage   83.68%   83.68%           
=======================================
  Files         346      346           
  Lines       19002    19015   +13     
=======================================
+ Hits        15901    15912   +11     
- Misses       3101     3103    +2
Impacted Files Coverage Δ
torch_geometric/loader/link_neighbor_loader.py 100.00% <ø> (ø)
torch_geometric/sampler/utils.py 80.51% <89.47%> (-0.08%) ⬇️
torch_geometric/sampler/neighbor_sampler.py 92.40% <100.00%> (+0.14%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@rusty1s rusty1s changed the title NeighborSampler: Sort according to time in local neighborhoods NeighborSampler: Sort local neighborhoods according to time Sep 23, 2022
ZenoTan
ZenoTan approved these changes Sep 23, 2022
View reviewed changes
Copy link
Member

@ZenoTan ZenoTan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

mananshah99
mananshah99 reviewed Sep 23, 2022
View reviewed changes
Copy link
Contributor

@mananshah99 mananshah99 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly LGTM. Happy to chat more about how to modify the GraphStore design to support CSC sorted by node time, but at the moment I think we can get around this by passing node_time to csc(). Lmkwyt!

test/loader/test_neighbor_loader.py Show resolved Hide resolved
test/loader/test_neighbor_loader.py Show resolved Hide resolved
torch_geometric/sampler/neighbor_sampler.py Show resolved Hide resolved
torch_geometric/sampler/utils.py Show resolved Hide resolved
wsad1
wsad1 approved these changes Sep 26, 2022
View reviewed changes
Copy link
Member

@wsad1 wsad1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding this.

torch_geometric/sampler/utils.py Outdated Show resolved Hide resolved
@rusty1s rusty1s merged commit e998348 into master Sep 29, 2022
@rusty1s rusty1s deleted the time_sort branch September 29, 2022 22:12
JakubPietrakIntel pushed a commit to JakubPietrakIntel/pytorch_geometric that referenced this pull request Nov 25, 2022
@rusty1s @JakubPietrakIntel
NeighborSampler: Sort local neighborhoods according to time (pyg-…
4bb0a9b 
…team#5516)

* update

* update

* update

* update doc-string

* changelog

* update

* update

* update
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mananshah99 mananshah99 mananshah99 left review comments

@ZenoTan ZenoTan ZenoTan approved these changes

@wsad1 wsad1 wsad1 approved these changes

Assignees

@rusty1s rusty1s

Labels
0 - Priority P0 feature loader
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@rusty1s @mananshah99 @wsad1 @ZenoTan