Skip to content
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

Recurrent layer to treat 2nd and the rest of inputs as initial_states #7691

Merged
merged 3 commits into from
Aug 24, 2017

Conversation

wanasit
Copy link
Contributor

@wanasit wanasit commented Aug 20, 2017

Hi. I'm submitting a PR for a bug reported in #7612.

I think the Recurrent layer should treat a part of the inputs as initial_states. I think this is also what you want to do according to the API doc. If you have a different design, please suggest.

@wanasit wanasit force-pushed the recurrent_inputs_as_init_states branch from aef0616 to 9601c67 Compare August 24, 2017 11:50
fchollet
fchollet previously approved these changes Aug 24, 2017
initial_state = [np.random.random((num_samples, units))
for _ in range(num_states)]
targets = np.random.random((num_samples, units))
model.fit([main_inputs] + initial_state, targets)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use train_on_batch to make the test lighter. Similarly I suggest you use 2 instead of num_samples

Copy link
Contributor Author

@wanasit wanasit Aug 24, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Switching to train_on_batch, but num_samples is already defined as 2 at the beginning of the file, isn't it.

@keras_test
def test_saving_recurrent_layer_with_init_state():
VECTOR_SIZE = 8
INPUT_LENGTH = 20
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need for all caps variable names here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changing to lower case

Copy link
Member

@fchollet fchollet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks

@fchollet fchollet merged commit d440e4b into keras-team:master Aug 24, 2017
@fchollet
Copy link
Member

I believe Conv2DLSTM would be in need of a similar fix. Can you look into it?

@wanasit
Copy link
Contributor Author

wanasit commented Aug 25, 2017

I've never used the class. If you are not hurry, I may look into it this weekend.

dschwertfeger added a commit to dschwertfeger/keras that referenced this pull request Oct 8, 2017
…github.com:dschwertfeger/keras into tensorboard-callback-embeddings-from-layer-outputs

* 'tensorboard-callback-embeddings-from-layer-outputs' of github.com:dschwertfeger/keras:
  Update for PPM format (keras-team#7750)
  Style fix
  Style fix
  Prepare 2.0.8 release.
  Update TFRecord example.
  Update backend docs.
  Style fix.
  Update README.md and setup.py.
  Minor optimizer consistency fixes.
  Recurrent layer to treat 2nd and the rest of inputs as initial_states (keras-team#7691)
  added 2 missing scripts to the README in the examples folder (keras-team#7734)
  Fixes and tests wrt step-wise training.
  Fix PEP8 (keras-team#7731)
  fix image_ocr example steps_per_epoch and validation_steps params (keras-team#7696)
  Remove requirement that batch_size divides len(x_train) in Variational Autoencoders examples. (keras-team#7716)
  Small fixes in training.py
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants