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

Crystal Support #396

Merged
merged 54 commits into from
Oct 3, 2019
Merged

Crystal Support #396

merged 54 commits into from
Oct 3, 2019

Conversation

nissy-dev
Copy link
Contributor

@nissy-dev nissy-dev commented Sep 18, 2019

DONE

  • implement MEGNet Model
  • support Material Project Dataset
  • refactoring converter method (each model has each converter method)
  • migrate CGCNN Model
  • confirm test
    • pass examples/test_example.sh
    • pass pytest -m "not (gpu or slow)" tests

The detail description is here

TODO

  • add test (WIP)
  • fix path for Material Project Dataset

README.md Outdated
@@ -127,6 +131,7 @@ The following datasets are currently supported:
- Tox21 [9]
- MoleculeNet [11]
- ZINC (only 250k dataset) [12, 13]
- Material Project (日付を入れる) [23]
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

@codecov-io
Copy link

codecov-io commented Sep 23, 2019

Codecov Report

Merging #396 into experimental_mp will decrease coverage by 7.03%.
The diff coverage is 57.57%.

@@                 Coverage Diff                 @@
##           experimental_mp     #396      +/-   ##
===================================================
- Coverage             91.3%   84.27%   -7.04%     
===================================================
  Files                  227      250      +23     
  Lines                10999    11951     +952     
===================================================
+ Hits                 10043    10072      +29     
- Misses                 956     1879     +923

@nissy-dev
Copy link
Contributor Author

nissy-dev commented Sep 23, 2019

I wrote tests for CGCNN & MEGNet. Currently, all tests pass the forward test,
but don't pass the backward test except test_megnet_readout.py.

Now, most of the backward test are commented out and
I investigate why the backward test fails.

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, let me check running the code.

"""

# get atom feature vector
# TODO: ここも適当なPATHに置き換える
Copy link
Member

Choose a reason for hiding this comment

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

could you change __init__ method get data_dir path in argument and load json & create feat_dict inside __init__ ?
Then we can re-use self.feat_dict in this method.

chainer_chemistry/models/megnet.py Outdated Show resolved Hide resolved
chainer_chemistry/models/megnet.py Show resolved Hide resolved
examples/mp/README.md Outdated Show resolved Hide resolved
examples/mp/README.md Outdated Show resolved Hide resolved
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.
This is experimental branch, so let me merge this PR at first.
I will proceed following:

  • include changes of current master branch (dataset class are added to support converter)
  • write download script of materials project dataset
    • modify data_dir path etc.
  • include citation/license properly
  • check code runs
  • merge to master branch

@corochann corochann merged commit c968b0f into chainer:experimental_mp Oct 3, 2019
@corochann corochann mentioned this pull request Oct 3, 2019
6 tasks
@corochann corochann mentioned this pull request Oct 23, 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.

6 participants