Fix dumps for Constant initializer #15150
Conversation
abhinavs95
changed the title
|
@mxnet-label-bot Add [pr-awaiting-review, NDArray] |
|
@szha @anirudhacharya Could you take a look? Thanks. |
|
@abhinavs95 can you please add a ut for this? |
|
@anirudhacharya added test. |
tests/python/unittest/test_init.py
Outdated
|
|
||
| def test_const_init_dumps(): | ||
| # test NDArray input | ||
| init = mx.init.Constant(mx.nd.ones(5)) |
There was a problem hiding this comment.
it might be good if we could randomize the shape and dimension for this test and test it for different dimensions
There was a problem hiding this comment.
Added randomized shape and dimension for the array in test, it can take values between 1 and 10 with 1D to 5D shapes.
tests/python/unittest/test_init.py
Outdated
|
|
||
| def test_const_init_dumps(): | ||
| # test NDArray input | ||
| init = mx.init.Constant(mx.nd.ones(5)) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
Is this PR good to go? @abhinavs95 @anirudhacharya |
|
@abhinavs95 is this PR good to go for merge? |
|
@karan6181 yes its good to go from my side |
|
Merged. Thanks for the fix @abhinavs95 |
* update dumps for const init * add test * fix for numpy input * randomize test array shape and dim * fix test * replace type with isinstance
Description
Fixes #12404
Override the dumps method for Constant initializer to take care of NDArray input.