-
Notifications
You must be signed in to change notification settings - Fork 16
Sarkars/batchnorm update #318
base: master
Are you sure you want to change the base?
Conversation
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.
Future-proofing for this change: NervanaSystems/ngraph#2046
@@ -912,6 +912,81 @@ TEST(NNOps, Conv2DBackpropInputNHWCWithDilation) { | |||
} | |||
} // end of op Conv2DBackpropInputNHWCWithDilation | |||
|
|||
// FusedBatchNorm : Forward pass, training = true | |||
// TODO fix this test | |||
TEST(NNOps, DISABLED_FusedBatchNormNHWCTrainTrue) { |
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.
This test does not pass. Sample output:
[ RUN ] NNOps.DISABLE_FusedBatchNormNHWCTrainTrue
2018-11-19 01:03:39.177831: I tensorflow/core/common_runtime/process_util.cc:69] Creating new thread pool wit
h default inter op setting: 2. Tune using inter_op_parallelism_threads for best performance.
/localdisk/sarkars/workspace1/tf_ngtf_7_mkl_1_12/ngraph-tf/test/test_utilities.h:126: Failure
Value of: rt
Actual: false
Expected: true
TF output 20.955995559692383
NG output 20.606725692749023
/localdisk/sarkars/workspace1/tf_ngtf_7_mkl_1_12/ngraph-tf/test/test_utilities.h:126: Failure
Value of: rt
Actual: false
Expected: true
TF output 21.971120834350586
NG output 21.604936599731445
[ FAILED ] NNOps.DISABLE_FusedBatchNormNHWCTrainTrue (125 ms)
ng_y = make_shared<ng::op::GetOutputElement>(ng_batch_norm, 0); | ||
ng_mean = make_shared<ng::op::GetOutputElement>(ng_batch_norm, 1); | ||
ng_variance = make_shared<ng::op::GetOutputElement>(ng_batch_norm, 2); | ||
shared_ptr<ngraph::Node> ng_y_out, ng_mean_out, ng_variance_out; |
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.
I must misunderstand about the training op orders. In ngraph, shouldn't the output order be {gamma, beta, input}
? Could you please explain this a little bit?
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.
So this PR was to sync with this PR in ngraph, which reorders batch norm: NervanaSystems/ngraph#2046 (comment)
But apparently that has been closed, and will come later, so I suppose we don't have to do anything now.
TESTNOW |
1 similar comment
TESTNOW |
Fail message:
|
One more comment: in ngraph core, it requires the input dimension >=2. I think it might be good if we add this as a confirmation constraint. |
Opening a PR for future ngraph BN API change: https://github.com/NervanaSystems/ngraph/pull/2046/files