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

[Model][Pytorch] GraphSAGE #403

Merged
merged 6 commits into from Feb 22, 2019

Conversation

Projects
None yet
4 participants
@hbsun2113
Copy link
Contributor

hbsun2113 commented Feb 21, 2019

Description

simple implemention of GraphSAGE.

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

@jermainewang jermainewang requested a review from ZiyueHuang Feb 21, 2019

@jermainewang jermainewang changed the title simple implemention of GraphSAGE [Model][Pytorch] GraphSAGE Feb 21, 2019

@jermainewang jermainewang requested a review from BarclayII Feb 21, 2019

@ZiyueHuang
Copy link
Member

ZiyueHuang left a comment

Thanks for your contribution!

Show resolved Hide resolved examples/pytorch/graphsage/graphsage.py Outdated
Show resolved Hide resolved examples/pytorch/graphsage/graphsage.py Outdated
@ZiyueHuang

This comment has been minimized.

Copy link
Member

ZiyueHuang commented Feb 21, 2019

Do you have any plan to implement other aggregators such as LSTM aggregator?

@hbsun2113 hbsun2113 closed this Feb 21, 2019

@hbsun2113 hbsun2113 reopened this Feb 21, 2019

@BarclayII
Copy link
Collaborator

BarclayII left a comment

Overall good. It would be even better if torch.nn modules are more utilized. See below for details.

Show resolved Hide resolved examples/pytorch/graphsage/graphsage.py Outdated
Show resolved Hide resolved examples/pytorch/graphsage/graphsage.py Outdated
Show resolved Hide resolved examples/pytorch/graphsage/graphsage.py Outdated
Show resolved Hide resolved examples/pytorch/graphsage/graphsage.py Outdated
Show resolved Hide resolved examples/pytorch/graphsage/graphsage.py
Show resolved Hide resolved examples/pytorch/graphsage/graphsage.py Outdated
Show resolved Hide resolved examples/pytorch/graphsage/graphsage.py Outdated
Show resolved Hide resolved examples/pytorch/graphsage/graphsage.py Outdated
Show resolved Hide resolved examples/pytorch/graphsage/graphsage.py Outdated
Show resolved Hide resolved examples/pytorch/graphsage/graphsage.py Outdated
@hbsun2113

This comment has been minimized.

Copy link
Contributor Author

hbsun2113 commented Feb 21, 2019

@BarclayII @ZiyueHuang @jermainewang
Thanks for your review! I have fixed the code as you suggest. Can you tell me how to update the pull request?

@jermainewang

This comment has been minimized.

Copy link
Member

jermainewang commented Feb 21, 2019

Can you tell me how to update the pull request?

@hbsun2113 Simply push commits normally to your graphsage branch and the PR will be updated accordingly.

@hbsun2113

This comment has been minimized.

Copy link
Contributor Author

hbsun2113 commented Feb 22, 2019

I have updated the code as @BarclayII and @ZiyueHuang suggest. Thx!

@jermainewang

This comment has been minimized.

Copy link
Member

jermainewang commented Feb 22, 2019

@BarclayII any further comments?

@jermainewang

This comment has been minimized.

Copy link
Member

jermainewang commented Feb 22, 2019

Have added you to the contributor list.

@BarclayII

This comment has been minimized.

Copy link
Collaborator

BarclayII commented Feb 22, 2019

The MLP class is still nested. No big deal though.

Thanks again for your contribution!

@BarclayII BarclayII merged commit 8c75017 into dmlc:master Feb 22, 2019

1 check passed

continuous-integration/jenkins/pr-merge This commit looks good
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.