-
Notifications
You must be signed in to change notification settings - Fork 12
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
LSTM Support #3
Comments
Might be better to try to support https://github.com/Element-Research/rnn/blob/master/FastLSTM.lua instead, since it has batched GEMMs. This LSTM implementation has batched GEMMs as well, but uses nngraph: https://github.com/karpathy/char-rnn/blob/master/model/LSTM.lua. You'd need a way to apply |
@Kaixhin So I guess we could just pluck them out form the respective sub-components of the LSTM or FastLSTM modules. For the first part, we could use the internal references to @bshillingford is right about how to do this for FastLSTM. If you make this extension of the rnn package, I will make nninit a default dependency of the rnn package. |
@nicholas-leonard I realised that the internal references were sufficient for The main issue with
|
@Kaixhin Maybe there's a way to more strongly decouple the modified tensors from the nn Modules. Re: passing initialisation schemes as strings, what do you think about putting the schemes into a namespace then accepting them directly as arguments? local nninit = require 'nninit'
local m = nn.Linear(5,5):wInit(nninit.orthogonal) where that's either the function itself or a wrapper object. Subjectively cleaner. You've done some decoupling already but as a private API. Publicising the private API (maybe in a sub-namespace?) for initialisation primitives might look something like: local nninit = require 'nninit'
local lstm_input = nn.Linear(input_size, rnn_size*4) -- input weights
for _, wt in ipairs(lstm_input.weight:split(1, rnn_size)) do
nninit.inits.xavier(wt)
end
local lstm_recur = nn.Linear(rnn_size, rnn_size*4) -- recurrent weights
for _, wt in ipairs(lstm_recur.weight:split(1, rnn_size)) do
nninit.inits.orthogonal(wt)
end Less concise, but more flexible... there might be a way to find a good middle ground here. Perhaps with a function as the arg you can generalise more: local lstm_recur = nn.Linear(rnn_size, rnn_size*4)
:init(function(m) return m.weight:narrow(1, 1, rnn_size) end, ...)
:init(function(m) return m.weight:narrow(1, rnn_size, rnn_size) end, ...)
:init(function(m) return m.weight:narrow(1, 2*rnn_size, rnn_size) end, ...)... where the first arg to local lstm_recur = nn.Linear(rnn_size, rnn_size*4)
:init(function(m) return unpack(m.weight:split(1, rnn_size)) end, ...) Having a single Thoughts on these random ideas? |
Would like to have Functions for the target are powerful but lengthy - is the indexing operator not general enough? In this case would have the indices (either in table or binary mask form) as the third and final argument to lstm.i2g
:init('w', nninit.kaiming({dist = 'uniform', gain = {gain = 'lrelu', leakiness = 0.3}}))
:init('b', nninit.constant(1), {{3*hiddenSize-1, 3*hiddenSize}})
|
Good points. Indexing should be general enough in most cases, but I'm I agree re Torch's philosophy, but I see your library as a way to address Sent from mobile
|
I would like to be convinced that this could let weights be treated more generally and hence overcome a shortcoming with Torch, but this seems tricky. For example, I do see the case for more arbitrary parameters and am happy to go ahead with One trivial way to do more complicated initialisations is to introduce a |
OK new API almost ready on the indexing branch. The
|
Example now updated and simply having |
Although this library works fine with
nngraph
, it would be good to also supportrnn
- specifically the LSTM module. Given the new API introduced with #2, how can the elements of the cell be initalised individually? Any feedback @nicholas-leonard?A notable reason to support this would be to implement the large forget gate bias introduced in:
The idea of
nninit
is to allow experimentation with initialisations/free maintainers from implementing "best practices".The text was updated successfully, but these errors were encountered: