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

[Feature] Reimplement Immutable graph index in DGL #342

Merged
merged 76 commits into from
Jan 17, 2019

Conversation

zheng-da
Copy link
Collaborator

@zheng-da zheng-da commented Jan 7, 2019

Description

This is to reimplement immutable graph index in DGL. This is a framework-agnostic implementation.

Checklist

Please feel free to remove inapplicable items for your PR.

  • The PR title starts with [$CATEGORY] (such as [Model], [Doc], [Feature]])
  • Changes are complete (i.e. I finished coding on this PR)
  • All changes have test coverage
  • Code is well-documented
  • To the my best knowledge, examples are either not affected by this change,
    or have been fixed to be compatible with this change
  • Related issue is referred in this PR

Changes

@zheng-da zheng-da changed the title Reimplement Immutable graph index in DGL [Feature] Reimplement Immutable graph index in DGL Jan 7, 2019
include/dgl/immutable_graph.h Outdated Show resolved Hide resolved
@zheng-da
Copy link
Collaborator Author

finally addressed all comments.

Copy link
Member

@jermainewang jermainewang left a comment

Choose a reason for hiding this comment

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

Final batch of comments. I didn't go through the sampler codes in immutable_graph.cc. Too much for this PR already. I suggest, later when @BarclayII works on the random walk stuff, try to go through that again and see what happens and could be merged. What do you think @BarclayII ?

python/dgl/graph_index.py Show resolved Hide resolved
src/graph/graph.cc Show resolved Hide resolved
src/graph/graph.cc Show resolved Hide resolved
src/graph/graph.cc Show resolved Hide resolved
src/graph/graph.cc Show resolved Hide resolved
src/graph/immutable_graph.cc Outdated Show resolved Hide resolved
src/graph/immutable_graph.cc Outdated Show resolved Hide resolved
src/graph/immutable_graph.cc Outdated Show resolved Hide resolved
src/graph/immutable_graph.cc Outdated Show resolved Hide resolved
src/graph/immutable_graph.cc Show resolved Hide resolved
@BarclayII
Copy link
Collaborator

Agreed. @GaiYu0 your layer sampling depends on this PR as well, right?

@GaiYu0
Copy link
Collaborator

GaiYu0 commented Jan 16, 2019

I am fine as long as there is no change in major interface.

@jermainewang
Copy link
Member

Approved. Thanks Da!

@zheng-da zheng-da merged commit 929742b into dmlc:master Jan 17, 2019
@jermainewang jermainewang mentioned this pull request Feb 18, 2019
26 tasks
@zheng-da zheng-da deleted the immutable_graph branch June 15, 2019 22:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants