3
\$\begingroup\$

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 X1 and X2 with size N1 x N2. The Manhattan distance of X1 and X2 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:

\$\endgroup\$
1
  • \$\begingroup\$ 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 \$\endgroup\$
    – PavelDev
    Commented Dec 10, 2021 at 12:49

2 Answers 2

3
\$\begingroup\$

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.

\$\endgroup\$
3
  • \$\begingroup\$ 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>. \$\endgroup\$
    – JimmyHu
    Commented Dec 11, 2021 at 18:05
  • \$\begingroup\$ 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. \$\endgroup\$
    – JimmyHu
    Commented Dec 11, 2021 at 18:28
  • 1
    \$\begingroup\$ 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. \$\endgroup\$
    – G. Sliepen
    Commented Dec 11, 2021 at 20:02
3
\$\begingroup\$

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.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.