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

[NN] Add TAGCN nn.module and example #788

Merged
merged 35 commits into from Aug 25, 2019
Merged

[NN] Add TAGCN nn.module and example #788

merged 35 commits into from Aug 25, 2019

Conversation

@classicsong
Copy link
Contributor

@classicsong classicsong commented Aug 23, 2019

Description

Add TAGCN nn.module on dgl.nn.pytorch
Add TAGCN example under example.pytorch
Add unitest for TAGCN

#756

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

yzh119 and others added 29 commits Aug 6, 2019
Add noting that the PinSage model example under
example/pytorch/recommendation only work with Python 3.6+
as its dataset loader depends on stanfordnlp package
which work only with Python 3.6+.
…ide.

1. make dgl.nn.xxx frame agnostic
2. make test.backend include dgl.nn modules
3. modify test_edge_softmax of test/mxnet/test_nn.py and
    test/pytorch/test_nn.py work on both CPU and GPU
1. clear all agnostic related code in dgl.nn
2. make test_graph_conv agnostic to cpu/gpu

* cora: ~0.812 (0.804-0.823) (paper: 0.833)
* citeseer: ~0.715 (paper: 0.714)
* pubmed: ~0.790 (paper: 0.811)
Copy link
Member

@yzh119 yzh119 Aug 23, 2019

Choose a reason for hiding this comment

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

Try tuning some hyper-parameters like weight_decay and see if the result is still lower then the number in TAGCN paper.

Loading

Copy link
Contributor Author

@classicsong classicsong Aug 24, 2019

Choose a reason for hiding this comment

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

I tried weight_decay and lr for pubmed dataset, there is minor performance improvement.
The new test result was updated.

Loading

@classicsong classicsong changed the title Add TAGCN nn.module and example [NN] Add TAGCN nn.module and example Aug 24, 2019
Copy link
Member

@yzh119 yzh119 left a comment

LGTM, need some minor revisions.

Loading


from tagcn import TAGCN

#from gcn import GCN
Copy link
Member

@yzh119 yzh119 Aug 24, 2019

Choose a reason for hiding this comment

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

Please remove these comments

Loading

If not None, applies an activation function to the updated node features.
Default: ``None``.
Attributes
Copy link
Member

@yzh119 yzh119 Aug 24, 2019

Choose a reason for hiding this comment

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

Describe attributes here.

Loading

self._in_feats = in_feats
self._out_feats = out_feats
self._k = k

Copy link
Member

@yzh119 yzh119 Aug 24, 2019

Choose a reason for hiding this comment

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

remove unnecessary empty lines.

Loading

@jermainewang
Copy link
Member

@jermainewang jermainewang commented Aug 24, 2019

Since Cora/Citeseer/Pubmed are so common and their training loop are essentially the same, is it possible to put these modules in one example?

Loading

@yzh119
Copy link
Member

@yzh119 yzh119 commented Aug 24, 2019

Yes we can create a "citation network" folder in examples.

Loading

@yzh119
Copy link
Member

@yzh119 yzh119 commented Aug 24, 2019

Also, I recommend change the name of this clas from TAGCN to something ended with Conv, to be consistent with other class in this module.

Loading

@classicsong
Copy link
Contributor Author

@classicsong classicsong commented Aug 25, 2019

Also, I recommend change the name of this clas from TAGCN to something ended with Conv, to be consistent with other class in this module.

Actually, in nn/pytorch/conv.py it is called TGConv.

Loading

@classicsong
Copy link
Contributor Author

@classicsong classicsong commented Aug 25, 2019

Since Cora/Citeseer/Pubmed are so common and their training loop are essentially the same, is it possible to put these modules in one example?

I don't think so. These modules/examples can also be applied to other datasets. They are not bounded only with these three datasets actually.

Loading

yzh119
yzh119 approved these changes Aug 25, 2019
Copy link
Member

@yzh119 yzh119 left a comment

I'm ok with this pr, go ahead and merge it.
The error in CI comes from shared memory: #755 . You can append an empty commit by git commit --allow-empty -m "trigger" to trigger another unittest.

Loading

python/dgl/nn/pytorch/conv.py Outdated Show resolved Hide resolved
Loading
@yzh119 yzh119 merged commit 11fb217 into dmlc:master Aug 25, 2019
1 check passed
Loading
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants