Skip to content
This repository has been archived by the owner on Nov 17, 2023. It is now read-only.

Fix dumps for Constant initializer #15150

Merged
merged 6 commits into from
Jul 19, 2019
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions python/mxnet/initializer.py
Original file line number Diff line number Diff line change
Expand Up @@ -464,6 +464,11 @@ def __init__(self, value):
def _init_weight(self, _, arr):
arr[:] = self.value

def dumps(self):
if not np.isscalar(self._kwargs['value']):
self._kwargs['value'] = self._kwargs['value'].asnumpy().tolist()
szha marked this conversation as resolved.
Show resolved Hide resolved
return json.dumps([self.__class__.__name__.lower(), self._kwargs])

@register
class Uniform(Initializer):
"""Initializes weights with random values uniformly sampled from a given range.
Expand Down
11 changes: 10 additions & 1 deletion tests/python/unittest/test_init.py
Original file line number Diff line number Diff line change
Expand Up @@ -67,10 +67,19 @@ def test_bilinear_init():
bili_1d = np.array([[1/float(4), 3/float(4), 3/float(4), 1/float(4)]])
bili_2d = bili_1d * np.transpose(bili_1d)
assert (bili_2d == bili_weight.asnumpy()).all()


def test_const_init_dumps():
# test NDArray input
init = mx.init.Constant(mx.nd.ones(5))
Copy link
Member

Choose a reason for hiding this comment

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

it might be good if we could randomize the shape and dimension for this test and test it for different dimensions

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added randomized shape and dimension for the array in test, it can take values between 1 and 10 with 1D to 5D shapes.

Copy link
Member

@anirudhacharya anirudhacharya Jun 7, 2019

Choose a reason for hiding this comment

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

in line 29 test_variable_init and other initializer tests are written differently. can we follow a similar pattern of declaring a sym variable and binding it to a module and initializing the params.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am just checking the function of dumps() in the test I have added, is creating a module still required? test_rsp_const_init in line 48 already does that for Constant initializers.

Copy link
Member

Choose a reason for hiding this comment

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

test_rsp_const_init does the following -
https://github.com/apache/incubator-mxnet/blob/master/tests/python/unittest/test_init.py#L52-L57

The reason I am asking this change is because I think that will be a more comprehensive test and will check the whole flow, and it will test the initializer the way it will actually be used in production. This change should enable serializing and deserializing an ndarray object.

Just check once with a committer who will merge this code. Everything else LGTM.

assert init.dumps() == '["constant", {"value": [1.0, 1.0, 1.0, 1.0, 1.0]}]'
# test scalar input
init = mx.init.Constant(1)
assert init.dumps() == '["constant", {"value": 1}]'

if __name__ == '__main__':
test_variable_init()
test_default_init()
test_aux_init()
test_rsp_const_init()
test_bilinear_init()
test_const_init_dumps()