-
Notifications
You must be signed in to change notification settings - Fork 313
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
incompatibility with nngraph's gmodule in evaluate mode #172
Comments
@justinmaojones I opened up a ticket here : torch/nngraph#109 . To my mind this should work. Will wait to see what nngraph guys have to say. |
@nicholas-leonard Any update on this? According to Ivo's reply it seems that @justinmaojones's workaround is the solution. |
@Kaixhin Yeah just make sure the output is copied. So it the network is shallow, copy the output using nn.Copy or whatnot. Ideally, rnn would do this for you. It could be done with a test that is executed the first time forward is called in evaluation mode. If the behaviour of the rnn with a nn.Copy appended to the end is the same as without, then no nn.Copy is necessary, otherwise it is. |
Broken (see Element-Research/rnn#172). Irrelevant with #46.
Running the following script:
will generate the following error:
The cause of this error seems to be found in lines 330-336 of gmodule.lua (commit ebde0bd), which I have posted below.
Whenever gmodule:updateOutput() is called, the inputs to each node are cleared. When running Recurrence:updateOutput() in evaluate mode, the previous output of Recurrence is set to self.outputs[self.step-1](see line 96 of Recurrence.lua in commit e77fe74), which points to the input of one of the nngraph forwardnodes, which will get cleared once gmodule:updateOutput() is called. This is only a problem in evaluate mode, because it does not use clones.
One way I managed to get around this was save a copy of the current output of Recurrence in a buffer. In other words:
The text was updated successfully, but these errors were encountered: