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

Numpy ndarray should be serialized as Python list. #10727

Merged
merged 3 commits into from
Jul 20, 2018

Conversation

yanboliang
Copy link
Contributor

@yanboliang yanboliang commented Jul 19, 2018

Summary

If there is numpy ndarray in a layer config, for example, set Constant(np.zeros(2,3)) as the initializer, the internal representation should be Tensor in backend(not discriminate numpy ndarray or Python list). When serializing it to JSON format, it should be regards as normal Python list. Otherwise, it will throw exception when deserialization(Refer #10718), since from_config doesn't know(doesn't need) how to deserialize numpy ndarray if there are extra message header.

Note: I have verify this fix works well for #10718.

Related Issues

#10718

PR Overview

  • This PR requires new unit tests [y/n] (make sure tests are included)
  • This PR requires to update the documentation [y/n] (make sure the docs are up-to-date)
  • This PR is backwards compatible [y/n]
  • This PR changes the current API [y/n] (all API changes need to be approved by fchollet)

@yanboliang
Copy link
Contributor Author

It seems the failure case is non-relevant, let me re-trigger it.

@yanboliang yanboliang closed this Jul 19, 2018
@yanboliang yanboliang reopened this Jul 19, 2018
@fchollet
Copy link
Collaborator

This assumes that the deserialization process will know about the expected array dtype.

Please add a unit test.

def test_saving_constant_initializer_with_numpy():
"""
Test saving and loading model of constant initializer with numpy ndarray as input.
"""
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please format the docstring like other docstrings, e.g.

"""Test saving and loading model that includes an initializer with numpy input.
"""

Copy link
Collaborator

@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, thank you.

I believe tf.keras would have the same issue as well. Please consider making the same PR in the TensorFlow repository.

@fchollet fchollet merged commit b88bbba into keras-team:master Jul 20, 2018
@yanboliang yanboliang deleted the saving branch July 20, 2018 18:37
@yanboliang
Copy link
Contributor Author

@fchollet Sure, will do it soon. Thanks.

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