Manhattan distance calculation between two images in C++

Question 1

This is a follow-up question for Two dimensional gaussian image generator in C++ and Image pixelwise operation function with multiple inputs in C++. For learning C++20 and researching purposes, I am attempting to implement a function manhattan_distance for calculating Manhattan distance between two images input. The foumula of Manhattan distance calculation is as below.

Given two two-dimensional inputs X₁ and X₂ with size N₁ x N₂. The Manhattan distance of X₁ and X₂ is defined as

$$ \begin{split} D_{Manhattan}(X_{1}, X_{2}) & = \left\|X_{1} - X_{2}\right\|_{1} \\ & = \sum_{k_{1} = 1}^{N_{1}} \sum_{k_{2} = 1}^{N_{2}} |{( X_{1}(k_{1}, k_{2}) - X_{2}(k_{1}, k_{2}) )}| \end{split} $$

The experimental implementation

Image template class implementation (image.h):

/* Developed by Jimmy Hu */
#ifndef Image_H
#define Image_H
#include <algorithm>
#include <array>
#include <cassert>
#include <chrono>
#include <complex>
#include <concepts>
#include <fstream>
#include <functional>
#include <iostream>
#include <iterator>
#include <list>
#include <numeric>
#include <string>
#include <type_traits>
#include <variant>
#include <vector>
namespace TinyDIP
{
 template <typename ElementT>
 class Image
 {
 public:
 Image() = default;
 Image(const std::size_t width, const std::size_t height):
 width(width),
 height(height),
 image_data(width * height) { }
 Image(const std::size_t width, const std::size_t height, const ElementT initVal):
 width(width),
 height(height),
 image_data(width * height, initVal) {}
 Image(const std::vector<ElementT>& input, std::size_t newWidth, std::size_t newHeight):
 width(newWidth),
 height(newHeight)
 {
 assert(input.size() == newWidth * newHeight);
 this->image_data = input; // Deep copy
 }
 Image(const std::vector<std::vector<ElementT>>& input)
 {
 this->height = input.size();
 this->width = input[0].size();
 for (auto& rows : input)
 {
 this->image_data.insert(this->image_data.end(), std::begin(input), std::end(input)); // flatten
 }
 return;
 }
 constexpr ElementT& at(const unsigned int x, const unsigned int y)
 { 
 checkBoundary(x, y);
 return this->image_data[y * width + x];
 }
 constexpr ElementT const& at(const unsigned int x, const unsigned int y) const
 {
 checkBoundary(x, y);
 return this->image_data[y * width + x];
 }
 constexpr std::size_t getWidth() const
 {
 return this->width;
 }
 constexpr std::size_t getHeight() const
 {
 return this->height;
 }
 constexpr auto getSize()
 {
 return std::make_tuple(this->width, this->height);
 }
 std::vector<ElementT> const& getImageData() const { return this->image_data; } // expose the internal data
 void print(std::string separator = "\t", std::ostream& os = std::cout) const
 {
 for (std::size_t y = 0; y < this->height; ++y)
 {
 for (std::size_t x = 0; x < this->width; ++x)
 {
 // Ref: https://isocpp.org/wiki/faq/input-output#print-char-or-ptr-as-number
 os << +this->at(x, y) << separator;
 }
 os << "\n";
 }
 os << "\n";
 return;
 }
 // Enable this function if ElementT = RGB
 void print(std::string separator = "\t", std::ostream& os = std::cout) const
 requires(std::same_as<ElementT, RGB>)
 {
 for (std::size_t y = 0; y < this->height; ++y)
 {
 for (std::size_t x = 0; x < this->width; ++x)
 {
 os << "( ";
 for (std::size_t channel_index = 0; channel_index < 3; ++channel_index)
 {
 // Ref: https://isocpp.org/wiki/faq/input-output#print-char-or-ptr-as-number
 os << +this->at(x, y).channels[channel_index] << separator;
 }
 os << ")" << separator;
 }
 os << "\n";
 }
 os << "\n";
 return;
 }
 friend std::ostream& operator<<(std::ostream& os, const Image<ElementT>& rhs)
 {
 const std::string separator = "\t";
 for (std::size_t y = 0; y < rhs.height; ++y)
 {
 for (std::size_t x = 0; x < rhs.width; ++x)
 {
 // Ref: https://isocpp.org/wiki/faq/input-output#print-char-or-ptr-as-number
 os << +rhs.at(x, y) << separator;
 }
 os << "\n";
 }
 os << "\n";
 return os;
 }
 Image<ElementT>& operator+=(const Image<ElementT>& rhs)
 {
 assert(rhs.width == this->width);
 assert(rhs.height == this->height);
 std::transform(image_data.cbegin(), image_data.cend(), rhs.image_data.cbegin(),
 image_data.begin(), std::plus<>{});
 return *this;
 }
 Image<ElementT>& operator-=(const Image<ElementT>& rhs)
 {
 assert(rhs.width == this->width);
 assert(rhs.height == this->height);
 std::transform(image_data.cbegin(), image_data.cend(), rhs.image_data.cbegin(),
 image_data.begin(), std::minus<>{});
 return *this;
 }
 Image<ElementT>& operator*=(const Image<ElementT>& rhs)
 {
 assert(rhs.width == this->width);
 assert(rhs.height == this->height);
 std::transform(image_data.cbegin(), image_data.cend(), rhs.image_data.cbegin(),
 image_data.begin(), std::multiplies<>{});
 return *this;
 }
 Image<ElementT>& operator/=(const Image<ElementT>& rhs)
 {
 assert(rhs.width == this->width);
 assert(rhs.height == this->height);
 std::transform(image_data.cbegin(), image_data.cend(), rhs.image_data.cbegin(),
 image_data.begin(), std::divides<>{});
 return *this;
 }
 bool operator==(const Image<ElementT>& rhs) const
 {
 /* do actual comparison */
 if (rhs.width != this->width ||
 rhs.height != this->height)
 {
 return false;
 }
 return rhs.image_data == this->image_data;
 }
 bool operator!=(const Image<ElementT>& rhs) const
 {
 return !(this == rhs);
 }
 Image<ElementT>& operator=(Image<ElementT> const& input) = default; // Copy Assign
 Image<ElementT>& operator=(Image<ElementT>&& other) = default; // Move Assign
 Image(const Image<ElementT> &input) = default; // Copy Constructor
 Image(Image<ElementT> &&input) = default; // Move Constructor
 private:
 std::size_t width;
 std::size_t height;
 std::vector<ElementT> image_data;
 void checkBoundary(const size_t x, const size_t y) const
 {
 assert(x < width);
 assert(y < height);
 }
 };
}
#endif

manhattan_distance template function implementation:

template<TinyDIP::arithmetic ElementT = double>
constexpr static ElementT manhattan_distance(const Image<ElementT>& input1, const Image<ElementT>& input2)
{
 is_size_same(input1, input2);
 return TinyDIP::recursive_reduce(TinyDIP::difference(input1, input2).getImageData(), ElementT{});
}

difference template function implementation:

template<TinyDIP::arithmetic ElementT = double>
constexpr static auto difference(const Image<ElementT>& input1, const Image<ElementT>& input2)
{
 return TinyDIP::abs(TinyDIP::subtract(input1, input2));
}

abs template function implementation:

template<TinyDIP::arithmetic ElementT = double>
constexpr static auto abs(const Image<ElementT>& input)
{
 return TinyDIP::pixelwiseOperation([](auto&& element) { return std::abs(element); }, input);
}

subtract template function implementation:

template<class InputT>
constexpr static Image<InputT> subtract(const Image<InputT>& input1, const Image<InputT>& input2)
{
 is_size_same(input1, input2);
 return TinyDIP::pixelwiseOperation(std::minus<>{}, input1, input2);
}
template<class InputT = RGB>
requires (std::same_as<InputT, RGB>)
constexpr static Image<InputT> subtract(const Image<InputT>& input1, const Image<InputT>& input2)
{
 is_size_same(input1, input2);
 Image<InputT> output(input1.getWidth(), input1.getHeight());
 for (std::size_t y = 0; y < input1.getHeight(); ++y)
 {
 for (std::size_t x = 0; x < input1.getWidth(); ++x)
 {
 for(std::size_t channel_index = 0; channel_index < 3; ++channel_index)
 {
 output.at(x, y).channels[channel_index] = 
 std::clamp(
 input1.at(x, y).channels[channel_index] - 
 input2.at(x, y).channels[channel_index],
 0,
 255);
 }
 }
 }
 return output;
}

pixelwiseOperation template function implementation:

template<typename Op, class InputT, class... Args, std::size_t unwrap_level = 1>
constexpr static auto pixelwiseOperation(Op op, const Image<InputT>& input1, const Args&... inputs)
{
 auto output = TinyDIP::Image(
 recursive_transform<unwrap_level>(
 [&](auto&& element1, auto&&... elements) 
 {
 return op(element1, elements...);
 },
 (input1.getImageData()),
 (inputs.getImageData())...),
 input1.getWidth(),
 input1.getHeight());
 return output;
}
template<class ExPo, typename Op, class InputT, std::size_t unwrap_level = 1>
requires (std::is_execution_policy_v<std::remove_cvref_t<ExPo>>)
constexpr static auto pixelwiseOperation(ExPo execution_policy, Op op, const Image<InputT>& input1)
{
 auto output = TinyDIP::Image(
 recursive_transform<unwrap_level>(
 execution_policy,
 [&](auto&& element1) 
 {
 return op(element1);
 },
 (input1.getImageData())),
 input1.getWidth(),
 input1.getHeight());
 return output;
}

Full Testing Code

Tests for manhattan_distance function:

#include <algorithm>
#include <array>
#include <cassert>
#include <cmath>
#include <concepts>
#include <execution>
#include <fstream>
#include <functional>
#include <iostream>
#include <iterator>
#include <numeric>
#include <ranges>
#include <string>
#include <type_traits>
#include <utility>
#include <vector>
using BYTE = unsigned char;
struct RGB
{
 BYTE channels[3];
};
using GrayScale = BYTE;
namespace TinyDIP
{
 #define is_size_same(x, y) {assert(x.getWidth() == y.getWidth()); assert(x.getHeight() == y.getHeight());}
 // Reference: https://stackoverflow.com/a/58067611/6667035
 template <typename T>
 concept arithmetic = std::is_arithmetic_v<T>;
 // recursive_depth function implementation
 template<typename T>
 constexpr std::size_t recursive_depth()
 {
 return 0;
 }
 template<std::ranges::input_range Range>
 constexpr std::size_t recursive_depth()
 {
 return recursive_depth<std::ranges::range_value_t<Range>>() + 1;
 }
 // recursive_reduce implementation
 // Reference: https://codereview.stackexchange.com/a/251310/231235
 template<class T, class ValueType, class Function = std::plus<ValueType>>
 constexpr auto recursive_reduce(const T& input, ValueType init, const Function& f)
 {
 return f(init, input);
 }
 template<std::ranges::range Container, class ValueType, class Function = std::plus<ValueType>>
 constexpr auto recursive_reduce(const Container& input, ValueType init, const Function& f = std::plus<ValueType>())
 {
 for (const auto& element : input) {
 auto result = recursive_reduce(element, ValueType{}, f);
 init = f(init, result);
 }
 return init;
 }
 // recursive_invoke_result_t implementation
 template<std::size_t, typename, typename>
 struct recursive_invoke_result { };
 template<typename T, typename F>
 struct recursive_invoke_result<0, F, T> { using type = std::invoke_result_t<F, T>; };
 template<std::size_t unwrap_level, typename F, template<typename...> typename Container, typename... Ts>
 requires (std::ranges::input_range<Container<Ts...>> &&
 requires { typename recursive_invoke_result<unwrap_level - 1, F, std::ranges::range_value_t<Container<Ts...>>>::type; })
 struct recursive_invoke_result<unwrap_level, F, Container<Ts...>>
 {
 using type = Container<typename recursive_invoke_result<unwrap_level - 1, F, std::ranges::range_value_t<Container<Ts...>>>::type>;
 };
 template<std::size_t unwrap_level, typename F, typename T>
 using recursive_invoke_result_t = typename recursive_invoke_result<unwrap_level, F, T>::type;
 // recursive_variadic_invoke_result_t implementation
 template<std::size_t, typename, typename, typename...>
 struct recursive_variadic_invoke_result { };
 template<typename F, class...Ts1, template<class...>class Container1, typename... Ts>
 struct recursive_variadic_invoke_result<1, F, Container1<Ts1...>, Ts...>
 {
 using type = Container1<std::invoke_result_t<F,
 std::ranges::range_value_t<Container1<Ts1...>>,
 std::ranges::range_value_t<Ts>...>>;
 };
 template<std::size_t unwrap_level, typename F, class...Ts1, template<class...>class Container1, typename... Ts>
 requires ( std::ranges::input_range<Container1<Ts1...>> &&
 requires { typename recursive_variadic_invoke_result<
 unwrap_level - 1,
 F,
 std::ranges::range_value_t<Container1<Ts1...>>,
 std::ranges::range_value_t<Ts>...>::type; }) // The rest arguments are ranges
 struct recursive_variadic_invoke_result<unwrap_level, F, Container1<Ts1...>, Ts...>
 {
 using type = Container1<
 typename recursive_variadic_invoke_result<
 unwrap_level - 1,
 F,
 std::ranges::range_value_t<Container1<Ts1...>>,
 std::ranges::range_value_t<Ts>...
 >::type>;
 };
 template<std::size_t unwrap_level, typename F, typename T1, typename... Ts>
 using recursive_variadic_invoke_result_t = typename recursive_variadic_invoke_result<unwrap_level, F, T1, Ts...>::type;
 template<typename OutputIt, typename NAryOperation, typename InputIt, typename... InputIts>
 OutputIt transform(OutputIt d_first, NAryOperation op, InputIt first, InputIt last, InputIts... rest) {
 while (first != last) {
 *d_first++ = op(*first++, (*rest++)...);
 }
 return d_first;
 }
 // recursive_transform for the multiple parameters cases (the version with unwrap_level)
 template<std::size_t unwrap_level = 1, class F, class Arg1, class... Args>
 constexpr auto recursive_transform(const F& f, const Arg1& arg1, const Args&... args)
 {
 if constexpr (unwrap_level > 0)
 {
 static_assert(unwrap_level <= recursive_depth<Arg1>(),
 "unwrap level higher than recursion depth of input");
 recursive_variadic_invoke_result_t<unwrap_level, F, Arg1, Args...> output{};
 transform(
 std::inserter(output, std::ranges::end(output)),
 [&f](auto&& element1, auto&&... elements) { return recursive_transform<unwrap_level - 1>(f, element1, elements...); },
 std::ranges::cbegin(arg1),
 std::ranges::cend(arg1),
 std::ranges::cbegin(args)...
 );
 return output;
 }
 else
 {
 return f(arg1, args...);
 }
 }
 template <typename ElementT>
 class Image
 {
 public:
 Image() = default;
 Image(const std::size_t width, const std::size_t height):
 width(width),
 height(height),
 image_data(width * height) { }
 Image(const std::size_t width, const std::size_t height, const ElementT initVal):
 width(width),
 height(height),
 image_data(width * height, initVal) {}
 Image(const std::vector<ElementT>& input, std::size_t newWidth, std::size_t newHeight):
 width(newWidth),
 height(newHeight)
 {
 assert(input.size() == newWidth * newHeight);
 this->image_data = input; // Deep copy
 }
 Image(const std::vector<std::vector<ElementT>>& input)
 {
 this->height = input.size();
 this->width = input[0].size();
 
 for (auto& rows : input)
 {
 this->image_data.insert(this->image_data.end(), std::begin(input), std::end(input)); // flatten
 }
 return;
 }
 constexpr ElementT& at(const unsigned int x, const unsigned int y)
 { 
 checkBoundary(x, y);
 return this->image_data[y * width + x];
 }
 constexpr ElementT const& at(const unsigned int x, const unsigned int y) const
 {
 checkBoundary(x, y);
 return this->image_data[y * width + x];
 }
 constexpr std::size_t getWidth() const
 {
 return this->width;
 }
 constexpr std::size_t getHeight() const
 {
 return this->height;
 }
 constexpr auto getSize()
 {
 return std::make_tuple(this->width, this->height);
 }
 std::vector<ElementT> const& getImageData() const { return this->image_data; } // expose the internal data
 void print(std::string separator = "\t", std::ostream& os = std::cout) const
 {
 for (std::size_t y = 0; y < this->height; ++y)
 {
 for (std::size_t x = 0; x < this->width; ++x)
 {
 // Ref: https://isocpp.org/wiki/faq/input-output#print-char-or-ptr-as-number
 os << +this->at(x, y) << separator;
 }
 os << "\n";
 }
 os << "\n";
 return;
 }
 // Enable this function if ElementT = RGB
 void print(std::string separator = "\t", std::ostream& os = std::cout) const
 requires(std::same_as<ElementT, RGB>)
 {
 for (std::size_t y = 0; y < this->height; ++y)
 {
 for (std::size_t x = 0; x < this->width; ++x)
 {
 os << "( ";
 for (std::size_t channel_index = 0; channel_index < 3; ++channel_index)
 {
 // Ref: https://isocpp.org/wiki/faq/input-output#print-char-or-ptr-as-number
 os << +this->at(x, y).channels[channel_index] << separator;
 }
 os << ")" << separator;
 }
 os << "\n";
 }
 os << "\n";
 return;
 }
 friend std::ostream& operator<<(std::ostream& os, const Image<ElementT>& rhs)
 {
 const std::string separator = "\t";
 for (std::size_t y = 0; y < rhs.height; ++y)
 {
 for (std::size_t x = 0; x < rhs.width; ++x)
 {
 // Ref: https://isocpp.org/wiki/faq/input-output#print-char-or-ptr-as-number
 os << +rhs.at(x, y) << separator;
 }
 os << "\n";
 }
 os << "\n";
 return os;
 }
 Image<ElementT>& operator+=(const Image<ElementT>& rhs)
 {
 assert(rhs.width == this->width);
 assert(rhs.height == this->height);
 std::transform(image_data.cbegin(), image_data.cend(), rhs.image_data.cbegin(),
 image_data.begin(), std::plus<>{});
 return *this;
 }
 Image<ElementT>& operator-=(const Image<ElementT>& rhs)
 {
 assert(rhs.width == this->width);
 assert(rhs.height == this->height);
 std::transform(image_data.cbegin(), image_data.cend(), rhs.image_data.cbegin(),
 image_data.begin(), std::minus<>{});
 return *this;
 }
 Image<ElementT>& operator*=(const Image<ElementT>& rhs)
 {
 assert(rhs.width == this->width);
 assert(rhs.height == this->height);
 std::transform(image_data.cbegin(), image_data.cend(), rhs.image_data.cbegin(),
 image_data.begin(), std::multiplies<>{});
 return *this;
 }
 Image<ElementT>& operator/=(const Image<ElementT>& rhs)
 {
 assert(rhs.width == this->width);
 assert(rhs.height == this->height);
 std::transform(image_data.cbegin(), image_data.cend(), rhs.image_data.cbegin(),
 image_data.begin(), std::divides<>{});
 return *this;
 }
 bool operator==(const Image<ElementT>& rhs) const
 {
 /* do actual comparison */
 if (rhs.width != this->width ||
 rhs.height != this->height)
 {
 return false;
 }
 return rhs.image_data == this->image_data;
 }
 bool operator!=(const Image<ElementT>& rhs) const
 {
 return !(this == rhs);
 }
 Image<ElementT>& operator=(Image<ElementT> const& input) = default; // Copy Assign
 Image<ElementT>& operator=(Image<ElementT>&& other) = default; // Move Assign
 Image(const Image<ElementT> &input) = default; // Copy Constructor
 Image(Image<ElementT> &&input) = default; // Move Constructor
 
 private:
 std::size_t width;
 std::size_t height;
 std::vector<ElementT> image_data;
 void checkBoundary(const size_t x, const size_t y) const
 {
 assert(x < width);
 assert(y < height);
 }
 };
 template<typename Op, class InputT, class... Args, std::size_t unwrap_level = 1>
 constexpr static auto pixelwiseOperation(Op op, const Image<InputT>& input1, const Args&... inputs)
 {
 auto output = TinyDIP::Image(
 recursive_transform<unwrap_level>(
 [&](auto&& element1, auto&&... elements) 
 {
 return op(element1, elements...);
 },
 (input1.getImageData()),
 (inputs.getImageData())...),
 input1.getWidth(),
 input1.getHeight());
 return output;
 }
 template<class ExPo, typename Op, class InputT, std::size_t unwrap_level = 1>
 requires (std::is_execution_policy_v<std::remove_cvref_t<ExPo>>)
 constexpr static auto pixelwiseOperation(ExPo execution_policy, Op op, const Image<InputT>& input1)
 {
 auto output = TinyDIP::Image(
 recursive_transform<unwrap_level>(
 execution_policy,
 [&](auto&& element1) 
 {
 return op(element1);
 },
 (input1.getImageData())),
 input1.getWidth(),
 input1.getHeight());
 return output;
 }
 template<class InputT>
 constexpr static Image<InputT> subtract(const Image<InputT>& input1, const Image<InputT>& input2)
 {
 is_size_same(input1, input2);
 return TinyDIP::pixelwiseOperation(std::minus<>{}, input1, input2);
 }
 template<class InputT = RGB>
 requires (std::same_as<InputT, RGB>)
 constexpr static Image<InputT> subtract(const Image<InputT>& input1, const Image<InputT>& input2)
 {
 is_size_same(input1, input2);
 Image<InputT> output(input1.getWidth(), input1.getHeight());
 for (std::size_t y = 0; y < input1.getHeight(); ++y)
 {
 for (std::size_t x = 0; x < input1.getWidth(); ++x)
 {
 for(std::size_t channel_index = 0; channel_index < 3; ++channel_index)
 {
 output.at(x, y).channels[channel_index] = 
 std::clamp(
 input1.at(x, y).channels[channel_index] - 
 input2.at(x, y).channels[channel_index],
 0,
 255);
 }
 }
 }
 return output;
 }
 template<TinyDIP::arithmetic ElementT = double>
 constexpr static auto abs(const Image<ElementT>& input)
 {
 return TinyDIP::pixelwiseOperation([](auto&& element) { return std::abs(element); }, input);
 }
 template<TinyDIP::arithmetic ElementT = double>
 constexpr static auto difference(const Image<ElementT>& input1, const Image<ElementT>& input2)
 {
 return TinyDIP::abs(TinyDIP::subtract(input1, input2));
 }
 template<TinyDIP::arithmetic ElementT = double>
 constexpr static ElementT manhattan_distance(const Image<ElementT>& input1, const Image<ElementT>& input2)
 {
 is_size_same(input1, input2);
 return TinyDIP::recursive_reduce(TinyDIP::difference(input1, input2).getImageData(), ElementT{});
 }
}
template<class T>
void manhattanDistanceTest()
{
 std::size_t N1 = 10, N2 = 10;
 TinyDIP::Image<T> test_input(N1, N2);
 for (std::size_t y = 1; y <= N2; y++)
 {
 for (std::size_t x = 1; x <= N1; x++)
 {
 test_input.at(y - 1, x - 1) = x * 10 + y & 1;
 }
 }
 T expected = 0;
 auto actual = TinyDIP::manhattan_distance(test_input, test_input);
 assert(actual == expected);
 auto test_input2 = test_input;
 test_input2.at(1, 1) = test_input2.at(1, 1) + 1;
 expected = 1;
 actual = TinyDIP::manhattan_distance(test_input, test_input2);
 std::string message = "expected: " + std::to_string(expected) + ",\tactual:" + std::to_string(actual) + '\n';
 std::cout << message;
 assert(actual == expected);
 return;
}
int main()
{
 manhattanDistanceTest<int>();
 manhattanDistanceTest<long>();
 manhattanDistanceTest<float>();
 manhattanDistanceTest<double>();
 return 0;
}

The output of the testing code above:

expected: 1, actual:1
expected: 1, actual:1
expected: 1.000000, actual:1.000000
expected: 1.000000, actual:1.000000

A Godbolt link is here.

TinyDIP on GitHub

All suggestions are welcome.

The summary information:

Which question it is a follow-up to?

Two dimensional gaussian image generator in C++ and

Image pixelwise operation function with multiple inputs in C++
What changes has been made in the code since last question?

The implementation of manhattan_distance function is the main idea here.
Why a new review is being asked for?

If there is any possible improvement, please let me know.

Question 2

Consider using a pair of iterators in constructor at line 41 (image.h). In general case, pass vector as [const] vector<T>[&] v is not good idea, because it restrict the type of allocator to std::allocator<T> only

Question 3

It's nice to see some of the code you've posted previously at Code Review come together.

Unnecessary use of `this->`

You almost never need to write this-> in C++. I would omit it if possible. An exception to the rule would be in the implementation of binary operators, where it's more clear and symmetric to write something like return this->value == rhs.value.

Unnecessary use of `TinyDIP::`

Inside namespace TinyDIP you don't need to use the prefix TinyDIP::. I would remove it, since if you ever want to change the name of the namespace, you then only have to do it in one line.

Consider using `std::span`

Since you tagged the question C++20, consider using std::span for things like the argument to the constructors of TinyDIP::Image. That way, the caller can have the image constructed from data that is either in a std::vector, std::array or anything else that has the elements sequentially in memory.

Incorrect constructor for vectors of vectors

In the constructor of TinyDIP::Image that takes a vector of vectors as an argument, you have a for-loop that has an iteration variable rows, but you never use that inside the loop.

Unnecessary wrapping of functions

In pixelwiseOperation(), you have the function op that you want to apply to all elements of the images. However, you wrap this inside a lambda before passing it to recursive_transform(). But the wrapper doesn't do anything, it just in turn calls op. So why not pass op directly? See below for an example.

Simplify variadic templates

You have two versions of pixelwiseOperation(), one which takes exactly 3 arguments, and one which takes 3 + a variable number of arguments. However, note that parameter packs are allowed to be empty, so you don't need the version which takes exactly 3 arguments at all. Furthermore, the only reason you need to have both input1 and inputs is to get the size of the first image. The rest of the code doesn't care about input1, but now you have to pass it to everything explicitly. Instead, I would suggest you remove input1 from the argument list, and use a helper function function to get the first element of the parameter pack, like so:

template<typename Arg, typename... Args>
constexpr static auto& first_of(Arg& first, Args&...) {
 return first;
}
template<typename Op, class... Args, std::size_t unwrap_level = 1>
constexpr static auto pixelwiseOperation(Op op, const Args&... inputs)
{
 return TinyDIP::Image(
 recursive_transform<unwrap_level>(
 op,
 inputs.getImageData()...
 ),
 first_of(inputs...).getWidth(),
 first_of(inputs...).getHeight()
 );
}

Since you don't care whether it's the first or last input you take the size of (they should all have the same after all), you could even use a trick like this:

(inputs, ...).getWidth(),
(inputs, ...).getHeight()

But this will produce lots of compiler warnings about left hand sides of comma operators having no effect.

The `unwrap_level` can't be changed

If you have a template parameter pack, any template parameters after that pack cannot be manually changed anymore. Furthermore, it's very cumbersome to first have to specify all other types first when those can be deduced from the input. So make sure unwrap_level is the first template parameter. Or in this case, you could also make it the only one, and use auto for the function arguments, like so:

template<std::size_t unwrap_level = 1>
constexpr static auto pixelwiseOperation(auto op, const auto&... inputs) {
 ...
}

If it shouldn't be changed to begin with, then unwrap_level should not be a template argument, instead just pass 1 directly as the template argument of the call to recursive_transform() (or don't specify it at all, since the latter function has it defaulted to 1 itself).

Consider adding `friend` arithmetic operators

You already have member operators like operator+=(), but there is no operator+(). Consider implementing them as well, as friend functions.

Incorrect behavior of `difference()`

The function difference() first calls subtract(), then calls abs() on it. However, if ElementT is an unsigned type, then the result of subtract() will always be positive, so abs() will do nothing. You have to calculate the absolute difference for each element indivually.

Question 4

Thank you for answering. About the mentioned issue of unwrap_level, this is interesting. The unwrap_level can't be changed because it is placed after a template parameter pack. Actually, the unwrap_level parameter isn't supposed to be changed. In other words, unlike recursive_transform function, unwrap_level should always be 1 in pixelwiseOperation because it is for iterating the elements get from Image.getImageData(), which returned a std::vector<ElementT>.

Question 5

By the way, there is an idea comes up in my mind. For those template parameters which are already placed after a template parameter pack, is it possible to access them with the way of matching arguments by name, not by position? I haven't seen the similar way been proposed. The existing Proposal of Designated Arguments is a little close, but not for template parameter case.

Question 6

If unwrap_level should be fixed to 1, it doesn't need to be a template parameter to begin with. As for the designated template arguments, it is mentioned in section 4.2 of the document you linked to, so they are thinking about it, but it looks like it won't be in C++23.

Question 7

Your only member data is a couple numbers and a vector, so you should not need to declare any copy constructor, destructor, etc. Use the rule of zero rather than declaring all the functions as =default explicitly.

Don't use this-> to access member data. The members of a class are in scope within the member function, and it's proper and idiomatic to just refer to them in the same way as local variables. Using this-> makes the reader think that something funny must be going on, as it is not normal nor expected.

Image_H is nowhere near unique! A real program will include multiple libraries with different developers, each including many files and other libraries. You are just asking for a collision here. If you use this kind of mechanism instead of #pragma once for portability, use a proper UUID for the symbol.

assert(input.size() == newWidth * newHeight);
Would an exception be better than terminating the whole program? Let the caller decide how bad this is. If this was triggered via a menu operation, you would expect nothing to have changed and go back to the GUI, not get knocked out of the program. That's very unfriendly. It also doesn't actually do anything in release build. Alternatively, you could automatically resize to match the other parameters.

Image(const std::vector<ElementT>& input, std::size_t newWidth, std::size_t newHeight):
 ⋮
 this->image_data = input; // Deep copy

Also, this is a perfect place to use a sink idiom. Take input by value and move it into image_data. That gives the caller the option to himself move the vector into the class; if he doesn't, it's exactly as before with a copy being made.

Image(const std::vector<std::vector<ElementT>>& input)

( How does that even compile? You used input instead of row for the source of the copy )

That's kind of odd. Is that something you actually have, and need to support? You're not checking that every row vector is the same width.

Don't write std::begin(x). You need the so-called "std two-step" to write generic code. If you want to assume that since it's a vector and you'll never change the class, just use the .begin member. If you do change the type at some point (very common maintenance thing), then as-written it will explicitly attempt to use std::begin which may not be found (so at least you know you'll have to change that, thanks to a compile-time error) or worse it will just not work correctly or not pick up the more optimized version written for that new type. With the namespace qualification, it will not use ADL.

Since you mentioned C++20, just use std::ranges::begin instead. Always. It fixes the issues with std::begin and automatically does the right thing.

G. Sliepen G. Sliepen 68.6k3 gold badges74 silver badges179 bronze badges · Accepted Answer · 2021-12-10 19:51:14Z

It's nice to see some of the code you've posted previously at Code Review come together.

Unnecessary use of `this->`

You almost never need to write this-> in C++. I would omit it if possible. An exception to the rule would be in the implementation of binary operators, where it's more clear and symmetric to write something like return this->value == rhs.value.

Unnecessary use of `TinyDIP::`

Inside namespace TinyDIP you don't need to use the prefix TinyDIP::. I would remove it, since if you ever want to change the name of the namespace, you then only have to do it in one line.

Consider using `std::span`

Since you tagged the question C++20, consider using std::span for things like the argument to the constructors of TinyDIP::Image. That way, the caller can have the image constructed from data that is either in a std::vector, std::array or anything else that has the elements sequentially in memory.

Incorrect constructor for vectors of vectors

In the constructor of TinyDIP::Image that takes a vector of vectors as an argument, you have a for-loop that has an iteration variable rows, but you never use that inside the loop.

Unnecessary wrapping of functions

In pixelwiseOperation(), you have the function op that you want to apply to all elements of the images. However, you wrap this inside a lambda before passing it to recursive_transform(). But the wrapper doesn't do anything, it just in turn calls op. So why not pass op directly? See below for an example.

Simplify variadic templates

You have two versions of pixelwiseOperation(), one which takes exactly 3 arguments, and one which takes 3 + a variable number of arguments. However, note that parameter packs are allowed to be empty, so you don't need the version which takes exactly 3 arguments at all. Furthermore, the only reason you need to have both input1 and inputs is to get the size of the first image. The rest of the code doesn't care about input1, but now you have to pass it to everything explicitly. Instead, I would suggest you remove input1 from the argument list, and use a helper function function to get the first element of the parameter pack, like so:

template<typename Arg, typename... Args>
constexpr static auto& first_of(Arg& first, Args&...) {
 return first;
}
template<typename Op, class... Args, std::size_t unwrap_level = 1>
constexpr static auto pixelwiseOperation(Op op, const Args&... inputs)
{
 return TinyDIP::Image(
 recursive_transform<unwrap_level>(
 op,
 inputs.getImageData()...
 ),
 first_of(inputs...).getWidth(),
 first_of(inputs...).getHeight()
 );
}

Since you don't care whether it's the first or last input you take the size of (they should all have the same after all), you could even use a trick like this:

(inputs, ...).getWidth(),
(inputs, ...).getHeight()

But this will produce lots of compiler warnings about left hand sides of comma operators having no effect.

The `unwrap_level` can't be changed

If you have a template parameter pack, any template parameters after that pack cannot be manually changed anymore. Furthermore, it's very cumbersome to first have to specify all other types first when those can be deduced from the input. So make sure unwrap_level is the first template parameter. Or in this case, you could also make it the only one, and use auto for the function arguments, like so:

template<std::size_t unwrap_level = 1>
constexpr static auto pixelwiseOperation(auto op, const auto&... inputs) {
 ...
}

If it shouldn't be changed to begin with, then unwrap_level should not be a template argument, instead just pass 1 directly as the template argument of the call to recursive_transform() (or don't specify it at all, since the latter function has it defaulted to 1 itself).

Consider adding `friend` arithmetic operators

You already have member operators like operator+=(), but there is no operator+(). Consider implementing them as well, as friend functions.

Incorrect behavior of `difference()`

The function difference() first calls subtract(), then calls abs() on it. However, if ElementT is an unsigned type, then the result of subtract() will always be positive, so abs() will do nothing. You have to calculate the absolute difference for each element indivually.

Thank you for answering. About the mentioned issue of unwrap_level, this is interesting. The unwrap_level can't be changed because it is placed after a template parameter pack. Actually, the unwrap_level parameter isn't supposed to be changed. In other words, unlike recursive_transform function, unwrap_level should always be 1 in pixelwiseOperation because it is for iterating the elements get from Image.getImageData(), which returned a std::vector<ElementT>.
By the way, there is an idea comes up in my mind. For those template parameters which are already placed after a template parameter pack, is it possible to access them with the way of matching arguments by name, not by position? I haven't seen the similar way been proposed. The existing Proposal of Designated Arguments is a little close, but not for template parameter case.
If unwrap_level should be fixed to 1, it doesn't need to be a template parameter to begin with. As for the designated template arguments, it is mentioned in section 4.2 of the document you linked to, so they are thinking about it, but it looks like it won't be in C++23.

Stack Exchange Network

Manhattan distance calculation between two images in C++

2 Answers 2

Unnecessary use of `this->`

Unnecessary use of `TinyDIP::`

Consider using `std::span`

Incorrect constructor for vectors of vectors

Unnecessary wrapping of functions

Simplify variadic templates

The `unwrap_level` can't be changed

Consider adding `friend` arithmetic operators

Incorrect behavior of `difference()`

Your Answer

Sign up or log in

Post as a guest

Post as a guest

Linked

Hot Network Questions

Manhattan distance calculation between two images in C++

2 Answers 2

Unnecessary use of this->

Unnecessary use of TinyDIP::

Consider using std::span

Incorrect constructor for vectors of vectors

Unnecessary wrapping of functions

Simplify variadic templates

The unwrap_level can't be changed

Consider adding friend arithmetic operators

Incorrect behavior of difference()

Your Answer

Sign up or log in

Post as a guest

Post as a guest

Linked

Related

Hot Network Questions

Unnecessary use of `this->`

Unnecessary use of `TinyDIP::`

Consider using `std::span`

The `unwrap_level` can't be changed

Consider adding `friend` arithmetic operators

Incorrect behavior of `difference()`