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

[WIP] [NN] Refactor NN package #406

Merged
merged 28 commits into from
Feb 25, 2019
Merged

[WIP] [NN] Refactor NN package #406

merged 28 commits into from
Feb 25, 2019

Conversation

jermainewang
Copy link
Member

@jermainewang jermainewang commented Feb 21, 2019

Description

This PR refactor the out-of-date NN package. Some guidelines:

  • We won't try to unify different backend for this. User needs to explicitly import, for example dgl.nn.pytorch. This is because the interface might be quite different for different backends.
  • Try to follow the convention of each backend.
  • Don't forget to add docstring.

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

  • dgl.nn.pytorch.conv.GraphConv
  • dgl.nn.pytorch.softmax.EdgeSoftmax
  • dgl.nn.mxnet.conv.GraphConv
  • Also change the example to use these predefined modules.

Additional Note

  • I've also done some small fixes on the way to modify the examples
  • The incorporation of nn modules into the examples seem to have slowed down the speed of execution as there are some redundant operations of setting and popping data.

@jermainewang jermainewang mentioned this pull request Feb 21, 2019
26 tasks
@mufeili
Copy link
Member

mufeili commented Feb 22, 2019

@jermainewang I assume only the examples will be affected and the tutorials are left unchanged?

@jermainewang
Copy link
Member Author

Yes.

@mufeili mufeili changed the title [WIP][NN] Refactor NN package [NN] Refactor NN package Feb 23, 2019
@mufeili mufeili changed the title [NN] Refactor NN package [WIP] [NN] Refactor NN package Feb 23, 2019
@mufeili
Copy link
Member

mufeili commented Feb 23, 2019

@jermainewang I'm done and you may take a look. There are some additional changes that I did not realize until now (the ones in sampler.h, sampler.cc, pylintrc and task_lint.sh), I suppose these are your changes?

@jermainewang
Copy link
Member Author

There are some additional changes that I did not realize until now (the ones in sampler.h, sampler.cc, pylintrc and task_lint.sh), I suppose these are your changes?

Yes

@jermainewang
Copy link
Member Author

Did some minor fix on MXNet side. Thanks Murphy!

@jermainewang jermainewang merged commit 565f0c8 into dmlc:master Feb 25, 2019
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.

2 participants