-
Notifications
You must be signed in to change notification settings - Fork 6.8k
Fix BatchNorm converter for CoreML when fix_gamma=True #13557
Conversation
@@ -445,7 +445,7 @@ def test_tiny_conv_random_input_multi_filter(self): | |||
self._test_mxnet_model(net, input_shape=input_shape, mode='random') | |||
|
|||
def test_conv_random(self): | |||
np.random.seed(1988) | |||
np.random.seed(1986) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wondering why you changed the seed value here from 1988 to 1986, is there any significance to this number?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because I wasn't able to pass the tests (test_conv_random
, test_transpose
) before changing any code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution!
@CyberZHG Could you please run the test for 5k times to make sure that its not flaky? Thanks! |
The results are always exactly the same after setting the seed values. I triggered the CI (uncomment the line in Besides, I've done some calculations manually for convolution and batch normalization, and the results are the same as mxnet's outputs. However, there are always some slight differences between coreml's outputs and mxnet's outputs in my machine. I think there may be some environmental differences and the new seeds are more compatible with different versions. In summary, currently all tests could pass the CI tasks and I can change back the seed values if it is insisted. |
@nswamy could you take a look at it? |
the final trained model. | ||
""" | ||
np.random.seed(1988) | ||
input_shape = (1, 2, 2, 3) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not use (1, 1, 2, 3) as in previous test_batch_norm()
tests? I think it can be helpful to choose the same input_shape to test the default behavior when fix_gamma is not specified. Ideally, the result should be the same as you supply fix_gamma=True.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some comment about unit test. otherwise LGTM. thanks for your contribution!
@apeforest I have modified the random seeds and the input shape with a new commit. The testing results can be seen at travis-ci. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Description
Force gamma values to be ones while converting batch normalization with
fix_gamma=True
in case of accidental changes of gamma (e.g. #9624).Checklist
Essentials