From 3c0de8948109ae2e72d808936db9380c7ab59fa1 Mon Sep 17 00:00:00 2001 From: thawn Date: Sun, 28 Apr 2024 16:13:50 +0200 Subject: [PATCH 1/3] fix the return type of multiply_image_and_position for images of type int8, multiply_image_and_position was limited to positions < 256 (and for int16 to positions < 2^16), because the return type was the same as the image type. However int8 images can have positions larger than that because position indices are of type int32. This commit ensures that the return type is at least int32 so that multiply_image_and_position can handle positions larger than the maximum integer for int8 and int16 images. closes issue tier1::multiply_image_and_position_func() implicitly assumes that the position has the same type as the input. #287 --- .../src/tier1/multiply_image_and_position.cpp | 16 +++++++++- .../test_multiply_image_and_position.cpp | 29 +++++++++++++++++++ 2 files changed, 44 insertions(+), 1 deletion(-) diff --git a/clic/src/tier1/multiply_image_and_position.cpp b/clic/src/tier1/multiply_image_and_position.cpp index d9279563..b12e1062 100644 --- a/clic/src/tier1/multiply_image_and_position.cpp +++ b/clic/src/tier1/multiply_image_and_position.cpp @@ -14,7 +14,21 @@ multiply_image_and_position_func(const Device::Pointer & device, Array::Pointer dst, int dimension) -> Array::Pointer { - tier0::create_like(src, dst); + auto type = src->dtype(); + switch (src->dtype()) + { + case dType::INT8: + case dType::INT16: + type = dType::INT32; + break; + case dType::UINT8: + case dType::UINT16: + type = dType::UINT32; + break; + default: + break; + } + tier0::create_like(src, dst, type); const KernelInfo kernel = { "multiply_image_and_position", kernel::multiply_image_and_position }; const ParameterList params = { { "src", src }, { "dst", dst }, { "index", dimension } }; const RangeArray range = { dst->width(), dst->height(), dst->depth() }; diff --git a/tests/tier1/test_multiply_image_and_position.cpp b/tests/tier1/test_multiply_image_and_position.cpp index fae1888a..0d3df330 100644 --- a/tests/tier1/test_multiply_image_and_position.cpp +++ b/tests/tier1/test_multiply_image_and_position.cpp @@ -30,6 +30,35 @@ TEST_P(TestMultiplyPixelAndCoord, execute) } } +TEST_P(TestMultiplyPixelAndCoord, returnType) +{ + std::string param = GetParam(); + cle::BackendManager::getInstance().setBackend(param); + auto device = cle::BackendManager::getInstance().getBackend().getDevice("", "all"); + device->setWaitToFinish(true); + + for (cle::dType type : { cle::dType::INT8, cle::dType::INT16 }) + { + auto gpu_input = cle::Array::create(5, 3, 1, 3, type, cle::mType::BUFFER, device); + auto gpu_output = cle::tier1::multiply_image_and_position_func(device, gpu_input, nullptr, 0); + EXPECT_EQ(gpu_output->dtype(), cle::dType::INT32); + } + + for (cle::dType type : { cle::dType::UINT8, cle::dType::UINT16 }) + { + auto gpu_input = cle::Array::create(5, 3, 1, 3, type, cle::mType::BUFFER, device); + auto gpu_output = cle::tier1::multiply_image_and_position_func(device, gpu_input, nullptr, 0); + EXPECT_EQ(gpu_output->dtype(), cle::dType::UINT32); + } + + for (cle::dType type : { cle::dType::FLOAT, cle::dType::UINT32, cle::dType::INT32 }) + { + auto gpu_input = cle::Array::create(5, 3, 1, 3, type, cle::mType::BUFFER, device); + auto gpu_output = cle::tier1::multiply_image_and_position_func(device, gpu_input, nullptr, 0); + EXPECT_EQ(gpu_output->dtype(), type); + } +} + std::vector getParameters() { From bec46a485aabea262f61ac9636453e888894f02c Mon Sep 17 00:00:00 2001 From: thawn Date: Wed, 1 May 2024 10:13:33 +0200 Subject: [PATCH 2/3] always return FLOAT that way, the output type is well defined and we do not risk rounding errors in functions that have floating point input, such as center_of_mass() --- .../src/tier1/multiply_image_and_position.cpp | 16 +------------ .../test_multiply_image_and_position.cpp | 24 +++++++------------ 2 files changed, 9 insertions(+), 31 deletions(-) diff --git a/clic/src/tier1/multiply_image_and_position.cpp b/clic/src/tier1/multiply_image_and_position.cpp index b12e1062..6688fa93 100644 --- a/clic/src/tier1/multiply_image_and_position.cpp +++ b/clic/src/tier1/multiply_image_and_position.cpp @@ -14,21 +14,7 @@ multiply_image_and_position_func(const Device::Pointer & device, Array::Pointer dst, int dimension) -> Array::Pointer { - auto type = src->dtype(); - switch (src->dtype()) - { - case dType::INT8: - case dType::INT16: - type = dType::INT32; - break; - case dType::UINT8: - case dType::UINT16: - type = dType::UINT32; - break; - default: - break; - } - tier0::create_like(src, dst, type); + tier0::create_like(src, dst, dType::FLOAT); const KernelInfo kernel = { "multiply_image_and_position", kernel::multiply_image_and_position }; const ParameterList params = { { "src", src }, { "dst", dst }, { "index", dimension } }; const RangeArray range = { dst->width(), dst->height(), dst->depth() }; diff --git a/tests/tier1/test_multiply_image_and_position.cpp b/tests/tier1/test_multiply_image_and_position.cpp index 0d3df330..8a944fd5 100644 --- a/tests/tier1/test_multiply_image_and_position.cpp +++ b/tests/tier1/test_multiply_image_and_position.cpp @@ -37,25 +37,17 @@ TEST_P(TestMultiplyPixelAndCoord, returnType) auto device = cle::BackendManager::getInstance().getBackend().getDevice("", "all"); device->setWaitToFinish(true); - for (cle::dType type : { cle::dType::INT8, cle::dType::INT16 }) + for (cle::dType type : { cle::dType::UINT8, + cle::dType::INT8, + cle::dType::UINT16, + cle::dType::INT16, + cle::dType::FLOAT, + cle::dType::UINT32, + cle::dType::INT32 }) { auto gpu_input = cle::Array::create(5, 3, 1, 3, type, cle::mType::BUFFER, device); auto gpu_output = cle::tier1::multiply_image_and_position_func(device, gpu_input, nullptr, 0); - EXPECT_EQ(gpu_output->dtype(), cle::dType::INT32); - } - - for (cle::dType type : { cle::dType::UINT8, cle::dType::UINT16 }) - { - auto gpu_input = cle::Array::create(5, 3, 1, 3, type, cle::mType::BUFFER, device); - auto gpu_output = cle::tier1::multiply_image_and_position_func(device, gpu_input, nullptr, 0); - EXPECT_EQ(gpu_output->dtype(), cle::dType::UINT32); - } - - for (cle::dType type : { cle::dType::FLOAT, cle::dType::UINT32, cle::dType::INT32 }) - { - auto gpu_input = cle::Array::create(5, 3, 1, 3, type, cle::mType::BUFFER, device); - auto gpu_output = cle::tier1::multiply_image_and_position_func(device, gpu_input, nullptr, 0); - EXPECT_EQ(gpu_output->dtype(), type); + EXPECT_EQ(gpu_output->dtype(), cle::dType::FLOAT); } } From c799a294cd9a10a20ec5b88316472bdef705cdd9 Mon Sep 17 00:00:00 2001 From: thawn Date: Thu, 2 May 2024 09:28:51 +0200 Subject: [PATCH 3/3] revert bounding box type fix we now rely on multiply_image_and_position() to return FLOAT values --- clic/src/tier3/bounding_box.cpp | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/clic/src/tier3/bounding_box.cpp b/clic/src/tier3/bounding_box.cpp index e0340a18..81e1e3a3 100644 --- a/clic/src/tier3/bounding_box.cpp +++ b/clic/src/tier3/bounding_box.cpp @@ -12,9 +12,7 @@ auto bounding_box_func(const Device::Pointer & device, const Array::Pointer & src) -> std::array { float min_x = 0, min_y = 0, min_z = 0, max_x = 0, max_y = 0, max_z = 0; - auto temp = Array::create( - src->width(), src->height(), src->depth(), src->dimension(), dType::INDEX, mType::BUFFER, src->device()); - tier1::multiply_image_and_position_func(device, src, temp, 0); + auto temp = tier1::multiply_image_and_position_func(device, src, nullptr, 0); max_x = tier2::maximum_of_all_pixels_func(device, temp); min_x = tier2::minimum_of_masked_pixels_func(device, temp, src); temp = tier1::multiply_image_and_position_func(device, src, nullptr, 1);