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

Chainer graph #398

Merged
merged 54 commits into from
Sep 25, 2019
Merged

Chainer graph #398

merged 54 commits into from
Sep 25, 2019

Conversation

knshnb
Copy link
Contributor

@knshnb knshnb commented Sep 19, 2019

New features

  • New data(set) patterns
    • sparse graph data(set)
    • padding graph data(set) with coo matrix
  • New models
    • sparse RelGCN
    • sparse GIN
    • GIN with coo matrix
  • New task
    • semi-supervised node classification on large graphs
  • New datasets
    • citation network (cora, citeseer)
    • reddit

Dataset class

Chemical Network
adjacency matrix NumpyTupleDataset PaddingGraphDataset
scatter operation SparseGraphDataset SparseGraphDataset
sparse matmul not supported PaddingGraphDataset (use_coo=True)

.gitignore Outdated Show resolved Hide resolved
chainer_chemistry/datasets/citation_network/citation.py Outdated Show resolved Hide resolved
chainer_chemistry/models/prediction/node_classifier.py Outdated Show resolved Hide resolved
device=-1,
converter=None,
use_default_extensions=True,
resume_path=None):
Copy link
Member

Choose a reason for hiding this comment

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

MEMO. TODO (nakago) later: check again. can't we merge to run_train?

examples/network_graph/README.md Show resolved Hide resolved
examples/qm9/train_qm9.py Outdated Show resolved Hide resolved
examples/network_graph/train_network_graph.py Show resolved Hide resolved
chainer_chemistry/models/ggnn.py Outdated Show resolved Hide resolved
chainer_chemistry/models/gin.py Show resolved Hide resolved
chainer_chemistry/models/gin.py Show resolved Hide resolved
chainer_chemistry/models/gin.py Outdated Show resolved Hide resolved
@corochann
Copy link
Member

updated comment

1 similar comment
@corochann
Copy link
Member

updated comment

@corochann
Copy link
Member

Thank you! LGTM. I will check to run example later!

@codecov-io
Copy link

Codecov Report

Merging #398 into master will decrease coverage by 6.81%.
The diff coverage is 27.85%.

@@            Coverage Diff             @@
##           master     #398      +/-   ##
==========================================
- Coverage   91.22%   84.41%   -6.82%     
==========================================
  Files         229      234       +5     
  Lines       11049    11449     +400     
==========================================
- Hits        10080     9665     -415     
- Misses        969     1784     +815

@corochann
Copy link
Member

corochann commented Sep 25, 2019

@knshnb
If you can add some test, please add test (coverage test is failing now, since new functionality is not tested.) But I am going to merge this PR, even though in current condition after I confirmed example code works.

Copy link
Member

@corochann corochann left a comment

Choose a reason for hiding this comment

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

LGTM!! I checked cora, citeseer, and reddit learning works (I did not check whether the accuracy is comparable to paper yet though...).

todo later.
Memo: I'd like to modify dataset methods to handle dirpath using os.path.join so that it works with/without / at last.


On the GPU:
```angular2html
PYTHONPATH=. python examples/network_graph/train_network_graph.py --dataset cora
Copy link
Member

Choose a reason for hiding this comment

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

I will add --device 0 later.

On the GPU:
```angular2html
PYTHONPATH=. python examples/network_graph/train_network_graph.py --dataset cora
```
Copy link
Member

Choose a reason for hiding this comment

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

todo later: I'd like to modify running this script from examples/network_graph instead of top directory.

Copy link
Member

Choose a reason for hiding this comment

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

todo later: I'd like to add --coo option explanation, which is necessary to run reddit dataset.

Copy link
Member

Choose a reason for hiding this comment

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

todo later: it seems gin_sparse and coo=true combination is not supported.

reddit dataset only works with --method gin and --coo true.

@corochann
Copy link
Member

I will merge this PR!

Known TODO: #400 #401

@corochann corochann merged commit 0aecad6 into chainer:master Sep 25, 2019
@corochann corochann added this to the 0.7.0 milestone Dec 9, 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.

3 participants