{"action":"created","issue":{"url":"https://api.github.com/repos/fchollet/keras/issues/4258","repository_url":"https://api.github.com/repos/fchollet/keras","labels_url":"https://api.github.com/repos/fchollet/keras/issues/4258/labels{/name}","comments_url":"https://api.github.com/repos/fchollet/keras/issues/4258/comments","events_url":"https://api.github.com/repos/fchollet/keras/issues/4258/events","html_url":"https://github.com/fchollet/keras/pull/4258","id":186653139,"number":4258,"title":"TimeDistributed masking(discussion, possible improvement)","user":{"login":"DeNeutoy","id":16001974,"avatar_url":"https://avatars.githubusercontent.com/u/16001974?v=3","gravatar_id":"","url":"https://api.github.com/users/DeNeutoy","html_url":"https://github.com/DeNeutoy","followers_url":"https://api.github.com/users/DeNeutoy/followers","following_url":"https://api.github.com/users/DeNeutoy/following{/other_user}","gists_url":"https://api.github.com/users/DeNeutoy/gists{/gist_id}","starred_url":"https://api.github.com/users/DeNeutoy/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/DeNeutoy/subscriptions","organizations_url":"https://api.github.com/users/DeNeutoy/orgs","repos_url":"https://api.github.com/users/DeNeutoy/repos","events_url":"https://api.github.com/users/DeNeutoy/events{/privacy}","received_events_url":"https://api.github.com/users/DeNeutoy/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":2,"created_at":"2016-11-01T21:31:08Z","updated_at":"2016-11-04T17:25:51Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/fchollet/keras/pulls/4258","html_url":"https://github.com/fchollet/keras/pull/4258","diff_url":"https://github.com/fchollet/keras/pull/4258.diff","patch_url":"https://github.com/fchollet/keras/pull/4258.patch"},"body":"This PR is intended as a discussion of the function of masking in the `TimeDistributed` wrapper, with some code to potentially demonstrate a different way it could work. It's certainly possible that we have mis-used `TimeDistributed` in a way that you have not intended - if this is the case, please just say and close this PR. \r\n\r\nWe have been using `TimeDistributed` in order to apply generic sequence encoders (eg LSTM, BOW) to a number of inputs across a particular dimension: `(num_samples, num_sentences, num_words, embedding_size) --> (num_samples, num_sentences, embedding_size)`. Specifically, the below problem arises because the function being `TimeDistributed` changes the dimension of its inputs from `(batch_size, num_words, embedding_size) --> (batch_size, embedding_size)`.\r\n\r\nIn this case, there are two issues: \r\n\r\n1) the mask is not passed to the `K.rnn` function applying the encoder each of the sentence inputs(where the time dimension here is num_sentences)\r\n\r\n2) the `TimeDistributed` layer just returns the input mask, which is no longer the correct shape: `(batch_size, num_sentences, num_words)`. Normally, you would assume that after a layer which encodes inputs in such a way, you would no longer need a mask - however, in this case, it is possible that different numbers of sentences are passed for a given input, meaning that we still require a mask of shape `num_samples, num_sentences` to guard against this.\r\n\r\n\r\nThe code in this PR suggests two things:\r\n\r\n1) The mask received by `TimeDistributed` should be passed to the `K.rnn` function. \r\n\r\n2) If  the dimension of the inputs is changed by the encoder, create a mask which is all ones unless for a given dimension, the input mask is all zeros (this addresses the fact that an entire sentence might be masked). One issue here is there is no way (I don't think) to check which dimensions have been reduced. This would be a possible reason to not implement this part of the PR, but we think that the first point certainly is relevant. The implementation in the `compute_mask` function of this PR assumes that if the dimension has changed by `n`, the output dimensions are just `input_shape[:-n]`. This is obviously not ideal. I've included it as a discussion point and if others can see a way to specify the dimensions which are reduced, I think this would be a good feature.\r\n\r\nThanks! Keras is great.\r\n\r\n@pdasigi \r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n"},"comment":{"url":"https://api.github.com/repos/fchollet/keras/issues/comments/258495298","html_url":"https://github.com/fchollet/keras/pull/4258#issuecomment-258495298","issue_url":"https://api.github.com/repos/fchollet/keras/issues/4258","id":258495298,"user":{"login":"DeNeutoy","id":16001974,"avatar_url":"https://avatars.githubusercontent.com/u/16001974?v=3","gravatar_id":"","url":"https://api.github.com/users/DeNeutoy","html_url":"https://github.com/DeNeutoy","followers_url":"https://api.github.com/users/DeNeutoy/followers","following_url":"https://api.github.com/users/DeNeutoy/following{/other_user}","gists_url":"https://api.github.com/users/DeNeutoy/gists{/gist_id}","starred_url":"https://api.github.com/users/DeNeutoy/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/DeNeutoy/subscriptions","organizations_url":"https://api.github.com/users/DeNeutoy/orgs","repos_url":"https://api.github.com/users/DeNeutoy/repos","events_url":"https://api.github.com/users/DeNeutoy/events{/privacy}","received_events_url":"https://api.github.com/users/DeNeutoy/received_events","type":"User","site_admin":false},"created_at":"2016-11-04T17:25:51Z","updated_at":"2016-11-04T17:25:51Z","body":"Would it be simpler to introduce a new argument to `TimeDistributed`, which is: `reduced_dimensions=None`. This would be a list/tuple of dimensions which your Layer reduces (automatically assumed to be none), which would allow the direct computation of the mask in `TimeDistributed` by computing `K.any(input_mask, reduced_dimensions)`, as this would return a mask value of 0 for that dimension iff there was no input. If there was input, the mask should be 1, as the masking would have already been handled by passing the `input_mask` to `K.rnn`. This would also be more naturally backward compatible. \r\n\r\nThoughts @matt-gardner, @fchollet?"}}
{"actor":{"display_login":"DeNeutoy"}}