From c36aaf86219f8a5ac2a6fecdc1022b9788d2d1aa Mon Sep 17 00:00:00 2001 From: Tomasz Jankowski Date: Wed, 18 Nov 2020 14:14:33 +0100 Subject: [PATCH] nGraph: Fix TopK output sorting by index (#3092) --- docs/ops/sort/TopK_1.md | 14 +++- docs/ops/sort/TopK_3.md | 16 +++- .../skip_tests_config.cpp | 3 +- .../include/ngraph/runtime/reference/topk.hpp | 44 +++-------- ngraph/test/backend/topk.in.cpp | 74 +++++++++++++++++++ 5 files changed, 108 insertions(+), 43 deletions(-) diff --git a/docs/ops/sort/TopK_1.md b/docs/ops/sort/TopK_1.md index ad42e139022..85ec6b4bf6a 100644 --- a/docs/ops/sort/TopK_1.md +++ b/docs/ops/sort/TopK_1.md @@ -10,7 +10,7 @@ * *axis* - * **Description**: Specifies the axis along which + * **Description**: Specifies the axis along which the values are retrieved. * **Range of values**: An integer. Negative value means counting dimension from the end. * **Type**: `int` * **Default value**: None @@ -50,7 +50,15 @@ Output tensor is populated by values computes in the following way: output[i1, ..., i(axis-1), j, i(axis+1) ..., iN] = top_k(input[i1, ...., i(axis-1), :, i(axis+1), ..., iN]), k, sort, mode) -So for each slice `input[i1, ...., i(axis-1), :, i(axis+1), ..., iN]` which represents 1D array, top_k value is computed individually. Sorting and minimum/maximum are controlled by `sort` and `mode` attributes. +So for each slice `input[i1, ...., i(axis-1), :, i(axis+1), ..., iN]` which represents 1D array, top_k value is computed individually. + +Sorting and minimum/maximum are controlled by `sort` and `mode` attributes: + * *mode*=`max`, *sort*=`value` - descending by value + * *mode*=`max`, *sort*=`index` - ascending by index + * *mode*=`max`, *sort*=`none` - undefined + * *mode*=`min`, *sort*=`value` - ascending by value + * *mode*=`min`, *sort*=`index` - ascending by index + * *mode*=`min`, *sort*=`none` - undefined **Example** @@ -76,4 +84,4 @@ So for each slice `input[i1, ...., i(axis-1), :, i(axis+1), ..., iN]` which repr -``` \ No newline at end of file +``` diff --git a/docs/ops/sort/TopK_3.md b/docs/ops/sort/TopK_3.md index 7636f66137f..7bf5c828cb6 100644 --- a/docs/ops/sort/TopK_3.md +++ b/docs/ops/sort/TopK_3.md @@ -10,7 +10,7 @@ * *axis* - * **Description**: Specifies the axis along which + * **Description**: Specifies the axis along which the values are retrieved. * **Range of values**: An integer. Negative value means counting dimension from the end. * **Type**: `int` * **Default value**: None @@ -31,7 +31,7 @@ * **Type**: `string` * **Default value**: None * **Required**: *yes* - + * *index_element_type* * **Description**: the type of output tensor with indices @@ -65,7 +65,15 @@ Output tensor is populated by values computes in the following way: output[i1, ..., i(axis-1), j, i(axis+1) ..., iN] = top_k(input[i1, ...., i(axis-1), :, i(axis+1), ..., iN]), k, sort, mode) -So for each slice `input[i1, ...., i(axis-1), :, i(axis+1), ..., iN]` which represents 1D array, *TopK* value is computed individually. Sorting and minimum/maximum are controlled by `sort` and `mode` attributes. +So for each slice `input[i1, ...., i(axis-1), :, i(axis+1), ..., iN]` which represents 1D array, *TopK* value is computed individually. + +Sorting and minimum/maximum are controlled by `sort` and `mode` attributes: + * *mode*=`max`, *sort*=`value` - descending by value + * *mode*=`max`, *sort*=`index` - ascending by index + * *mode*=`max`, *sort*=`none` - undefined + * *mode*=`min`, *sort*=`value` - ascending by value + * *mode*=`min`, *sort*=`index` - ascending by index + * *mode*=`min`, *sort*=`none` - undefined **Example** @@ -97,4 +105,4 @@ So for each slice `input[i1, ...., i(axis-1), :, i(axis+1), ..., iN]` which repr -``` \ No newline at end of file +``` diff --git a/inference-engine/tests/functional/plugin/cpu/shared_tests_instances/skip_tests_config.cpp b/inference-engine/tests/functional/plugin/cpu/shared_tests_instances/skip_tests_config.cpp index 81cb9ce2d95..08f41bf5227 100644 --- a/inference-engine/tests/functional/plugin/cpu/shared_tests_instances/skip_tests_config.cpp +++ b/inference-engine/tests/functional/plugin/cpu/shared_tests_instances/skip_tests_config.cpp @@ -51,8 +51,7 @@ std::vector disabledTestPatterns() { // TODO: Issue: 37862 R"(.*ReverseSequenceLayerTest.*netPRC=(I8|U8).*)", // TODO: Issue: 38841 - R"(.*TopKLayerTest.*k=10.*mode=min.*sort=index.*)", - R"(.*TopKLayerTest.*k=5.*sort=(none|index).*)", + R"(.*TopKLayerTest.*k=5.*sort=none.*)", // TODO: Issue: 43314 R"(.*Broadcast.*mode=BIDIRECTIONAL.*inNPrec=BOOL.*)", // TODO: Issue 43417 sporadic issue, looks like an issue in test, reproducible only on Windows platform diff --git a/ngraph/core/reference/include/ngraph/runtime/reference/topk.hpp b/ngraph/core/reference/include/ngraph/runtime/reference/topk.hpp index 4faac42c949..aed34f70d4a 100644 --- a/ngraph/core/reference/include/ngraph/runtime/reference/topk.hpp +++ b/ngraph/core/reference/include/ngraph/runtime/reference/topk.hpp @@ -58,17 +58,10 @@ namespace ngraph return a < b; } - template - inline bool sort_indices_descending(const std::tuple& a, - const std::tuple& b) - { - return std::get<1>(a) < std::get<1>(b); - } - template inline bool sort_indices_ascending(const std::tuple& a, const std::tuple& b) { - return std::get<1>(a) > std::get<1>(b); + return std::get<1>(a) < std::get<1>(b); } template @@ -133,35 +126,18 @@ namespace ngraph compare_min); } // Write temp vector to output - if (compute_max) + switch (sort) { - switch (sort) - { - case op::v1::TopK::SortType::NONE: break; - case op::v1::TopK::SortType::SORT_INDICES: - std::sort(workspace.begin(), - workspace.begin() + k, - sort_indices_descending); - break; - case op::v1::TopK::SortType::SORT_VALUES: + case op::v1::TopK::SortType::NONE: break; + case op::v1::TopK::SortType::SORT_INDICES: + std::sort( + workspace.begin(), workspace.begin() + k, sort_indices_ascending); + break; + case op::v1::TopK::SortType::SORT_VALUES: + if (compute_max) std::sort(workspace.begin(), workspace.begin() + k, compare_max); - break; - } - } - else - { - switch (sort) - { - case op::v1::TopK::SortType::NONE: break; - case op::v1::TopK::SortType::SORT_INDICES: - std::sort(workspace.begin(), - workspace.begin() + k, - sort_indices_ascending); - break; - case op::v1::TopK::SortType::SORT_VALUES: + else std::sort(workspace.begin(), workspace.begin() + k, compare_min); - break; - } } for (size_t j = 0; j < k; j++) { diff --git a/ngraph/test/backend/topk.in.cpp b/ngraph/test/backend/topk.in.cpp index 6218a430722..e61451b8bc1 100644 --- a/ngraph/test/backend/topk.in.cpp +++ b/ngraph/test/backend/topk.in.cpp @@ -30,6 +30,8 @@ #include "ngraph/runtime/tensor.hpp" #include "runtime/backend.hpp" #include "util/all_close_f.hpp" +#include "util/engine/test_engines.hpp" +#include "util/test_case.hpp" #include "util/test_control.hpp" #include "util/test_tools.hpp" @@ -37,6 +39,7 @@ using namespace std; using namespace ngraph; static string s_manifest = "${MANIFEST}"; +using TestEngine = test::ENGINE_CLASS_NAME(${BACKEND_NAME}); template bool compare_set(const vector& a, vector b) @@ -1265,3 +1268,74 @@ NGRAPH_TEST(${BACKEND_NAME}, topk_v1_invalid_k) const auto k_negative = op::Constant::create(element::i8, Shape{}, {-1}); EXPECT_THROW(op::v1::TopK(data, k_negative, 0, "max", "index"), ngraph::NodeValidationFailure); } + +template +class topk_backend : public ::testing::Test +{ +}; +TYPED_TEST_CASE_P(topk_backend); + +template +struct TopkSortTestOutputs +{ + Mode mode; + SortType sort_type; + std::vector output0; + std::vector output1; + + TopkSortTestOutputs(Mode mode, + SortType sort_type, + std::vector&& output0, + std::vector&& output1) + : mode(mode) + , sort_type(sort_type) + , output0(std::move(output0)) + , output1(std::move(output1)) + { + } +}; + +TYPED_TEST_P(topk_backend, topk_mode_sort_order) +{ + const Shape shape{5}; + const Shape rshape{3}; + const auto data = make_shared(element::f32, shape); + const auto k = op::Constant::create(element::i64, {}, {3}); + const int64_t axis = 0; + + // helpers to reduce code verbosity + using m = typename TypeParam::Mode; + using st = typename TypeParam::SortType; + using v_f = std::vector; + using v_i = std::vector; + + const std::vector input{3, 1, 2, 5, 4}; + + std::vector> valid_outputs; + valid_outputs.emplace_back(m::MAX, st::SORT_VALUES, v_f{5, 4, 3}, v_i{3, 4, 0}); + valid_outputs.emplace_back(m::MAX, st::SORT_INDICES, v_f{3, 5, 4}, v_i{0, 3, 4}); + valid_outputs.emplace_back(m::MIN, st::SORT_VALUES, v_f{1, 2, 3}, v_i{1, 2, 0}); + valid_outputs.emplace_back(m::MIN, st::SORT_INDICES, v_f{3, 1, 2}, v_i{0, 1, 2}); + + for (const auto& v : valid_outputs) + { + auto topk = make_shared(data, k, axis, v.mode, v.sort_type); + auto f0 = make_shared(OutputVector{topk->output(0)}, ParameterVector{data}); + auto f1 = make_shared(OutputVector{topk->output(1)}, ParameterVector{data}); + + auto test_case0 = test::TestCase(f0); + test_case0.add_input(input); + test_case0.add_expected_output(rshape, v.output0); + test_case0.run(); + + auto test_case1 = test::TestCase(f1); + test_case1.add_input(input); + test_case1.add_expected_output(rshape, v.output1); + test_case1.run(); + } +} + +REGISTER_TYPED_TEST_CASE_P(topk_backend, topk_mode_sort_order); + +typedef ::testing::Types TopKTypes; +INSTANTIATE_TYPED_TEST_CASE_P(${BACKEND_NAME}, topk_backend, TopKTypes);