tobecontinued commented on a change in pull request #17754:
URL: https://github.com/apache/incubator-mxnet/pull/17754#discussion_r419465944
##########
File path: tests/cpp/operator/runner/core_op_runner_test.cc
##########
@@ -46,7 +46,15 @@ static const std::vector<std::pair<std::string,
std::string>> test_unary_operato
static const std::vector<std::pair<std::string, std::string>>
test_binary_operators = {
{ "elemwise_add", "_backward_add" },
- { "elemwise_mul", "_backward_mul" }
+ // TODO(Deng, Wenqi): In
https://github.com/apache/incubator-mxnet/pull/17754,
+ // we have changed backward op to graph of ops for computing backward of
elemwise_mul,
+ // but the CoreOpExecutor in tests/cpp/include/test_core_op.h actually has
issues
+ // to support this way even it provides CoreOpExecutor::GetBackward for the
case.
+ // e.g: It actually assumes there is one backward for all kinds of op, but
elemwise_mul has two.
+ // It will get wrong dependency for the second backward in
CoreOpExecutor::GetBackwardDependency
+ // due to "return igrad_entries[0].node;" // and failed to call
CoreOpExecutor::bwd_inputs()
+ // and CoreOpExecutor::bwd_outpuss() due to "CHECK_EQ(backward_.size(),
1U)";.
+// { "elemwise_mul", "_backward_mul" }
Review comment:
For more explanation, becaue the failure of the test cases are caused
by defects of CoreOpExecutor, I just want a pr to achieve only one task. Maybe
we need a other pr or jira to fix the issuee of CoreOpExecutor.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
