Peephole connections #1207
Peephole connections #1207
Conversation
|
To calculate a next cell state, most of forward calculation are done out of the forward function. The next cell state is passed through the connection between the cell and output gate, and I include it to "inputs" as "peep_in_o". |
|
I've finished to implement links/connection/peephole.py. In addition, I've confirmed this code runs correctly by making tests/chainer_tests/links_tests/connection_tests/test_peephole.py. However, test_peephole.py is not completed because I've not implemented GPU version yet. |
|
I've implemented GPU test. Now I've finished to implement Peephole connections including test codes except explanation. I'm going to include the document soon. |
| from chainer.links.loss import hierarchical_softmax | ||
| from chainer.links.loss import negative_sampling | ||
| from chainer.links.model import classifier | ||
| from chainer.links.normalization import batch_normalization | ||
|
|
||
|
|
| self.assertIsInstance(self.link.c.data, self.link.xp.ndarray) | ||
| self.assertIsInstance(self.link.h.data, self.link.xp.ndarray) | ||
| self.link.to_gpu() | ||
| self.assertIsNot(self.link.xp, numpy) |
There was a problem hiding this comment.
assertIs(self.link.xp, cupy) is better
There was a problem hiding this comment.
This is something I want to ask you. At first, I wrote assertIs(self.link.xp, cupy) and of course import cupy at the top . However, when I pushed the code, Travis CI rejected it outputting an error message Cupy is not correctly installed . The detailed image is as below. What should I do?
|
@KotaroSetoyama I updated comments. Please check them. |
|
@delta2323 Thank you for your check. I modified all parts except |
|
I believe this code should work. Can I answer to your question? |
|
Thank you for your advice. I confirmed the code worked without errors. |
|
I found mistakes in the document and modified. |
|
Thank you for your considerable work. I checked your code and believe that it will work well. But to be honest, I cannot decide to merge this PR because I am wondering how to define the interface of RNN units as I submitted the issue. I am cautious about defining the interface because we must support backward compatibility. The situation is same as other types of RNN units like (SGU/DSGU and MGU). I am sorry for you because it is not the problem of your implementation but Chainer side's one. Once we find the solution, we are ready for merging your PR. I am appreciate you if you let me know any idea or we can discuss the matter with you. |
|
Could you rename the class from |
|
Also, could merge or rebase the master branch? |
|
I renamed the class name "Peephole" to "StatefulPeepholeLSTM" |
| class TestPeephole(unittest.TestCase): | ||
|
|
||
| def setUp(self): | ||
| self.link = links.Peephole(self.in_size, self.out_size) |
|
Please register |
|
@KotaroSetoyama Thank you for your quick response. LGTM except the comments. |
| @@ -38,6 +39,7 @@ | |||
| StatelessLSTM = lstm.StatelessLSTM | |||
| MLPConvolution2D = mlp_convolution_2d.MLPConvolution2D | |||
| Parameter = parameter.Parameter | |||
| Peephole = peephole.StatefulPeepholeLSTM | |||
|
LGTM 👍 |
I implemented forward and backward of LSTM with peephole connections. GPU codes has not been implemented yet. I implemented chainer/links/connection/peephole.py and chainer/functions/activation/peephole.py referring to chainer/links/connection/lstm.py and chainer/functions/activation/lstm.py. As you can see, comment out are the same to lstm.py's one, but I'll modify it later. To calculate output gate in links, next cell state are calculated twice in links and functions. This is waste of resources. I want to improve it.