chainer / chainer-chemistry

Chainer Chemistry: A Library for Deep Learning in Biology and Chemistry

MIT License

622 stars 129 forks source link

Implement SparseGGNN #342

Closed ir5 closed 5 years ago

ir5 commented 5 years ago

This PR adds new implementation: GGNN for sparse matrix inputs.

I slightly modified the implementation of GGNNUpdater. Now the updater can accept CooMatrix as well as dense ndarray.
The original GGNN is unchanged.
SparseGGNN is newly introduced.
Several tests are added.

corochann commented 5 years ago

Could you fix tests?

Below chainer<=4, CooMatrix does not exist, so please just skip test in that case. Error message: AttributeError: module 'chainer.utils' has no attribute 'CooMatrix'

For python 3.5, seems numpy produces error... Could you check it?

df = np.diff(sorted_pos, prepend=sorted_pos[0]) != 0
E       TypeError: diff() got an unexpected keyword argument 'prepend'

corochann commented 5 years ago

I would like to merge it after test is fixed. After merged, I would like to further refactor sparse model support as follows if possible:

make preprocessor produces CooMatrix
support converter to make batch for CooMatrix
Sparse model just receives batched CooMatrix, instead of separated data, row, col.

ir5 commented 5 years ago

I modified the code to skip tests of sparse models when required modules are not available.

It seems that CI build have failed because of test_standard_scaler.py. Why? StandardScaler must be irrelevant with this PR.

ir5 commented 5 years ago

I understood, some behavior has been changed since numpy==1.16.3 and thus serializers.load_npz is not working properly. https://github.com/pfnet-research/chainer-chemistry/pull/349#issuecomment-487424549

corochann commented 5 years ago

https://github.com/pfnet-research/chainer-chemistry/pull/347/commits/f690a76e24e853e1a4177ed0336863b34be7f499

I think numpy behavior has changed since 1.16.3, and chainer serializer is not working well now.

codecov-io commented 5 years ago

Codecov Report

Merging #342 into master will increase coverage by 1.01%. The diff coverage is 91.39%.

@@            Coverage Diff            @@
##           master    #342      +/-   ##
=========================================
+ Coverage   80.88%   81.9%   +1.01%     
=========================================
  Files         207     212       +5     
  Lines        9496    9868     +372     
=========================================
+ Hits         7681    8082     +401     
+ Misses       1815    1786      -29