8
\$\begingroup\$

I just wrote my first standard neural network with SGD gradient descent in c++, I am really interested if I have done anything wrong or inefficient, suggestions would help me a ton (There is lots of CSV reading/writing code, because I like to plug CSV's to a website which converts CSV to Line graph to see how is my network learning):

#pragma once
#include <vector>
#include <array>
#include <numeric>
#include <iostream>
#include <functional>
#include <fstream>
#include <math.h>

#define WEIGHT_RANDOM_RANGE .1

template<typename T>
struct ActivationFunction
{
    std::function<T(T)> activation;
    std::function<T(T)> derivative;
};

template<typename T>
struct ActivationFunctions
{
    inline static const ActivationFunction<T> relu = { [](T x) { return (x > T(0)) ? x : 0; }, [](T x) { return T(x > 0); } };
    inline static const ActivationFunction<T> sigm = { [](T x) { return T(1) / (T(1) + exp(-x)); }, [](T x) { T s = T(1) / (T(1) + exp(-x)); return s * (T(1) - s); } };
    inline static const ActivationFunction<T> tanh = { [](T x) { return std::tanh(x); }, [](T x) { T t = std::tanh(x); return T(1.0f) - t * t; } };
    inline static const ActivationFunction<T> step = { [](T x) { return x <= T(0) ? T(-1) : T(1); }, [](T x) { return 1; } };
    inline static const ActivationFunction<T> none = { [](T x) { return x; }, [](T x) { return 1; } };
};

template<typename T = float>
class NeuralNetwork
{
    using scalar = T;
    using layer = std::vector<scalar>;
    using neuron_layers = std::vector<layer>;
    using batch = neuron_layers;
    using neuron_weights = std::vector<neuron_layers>;
    using size_type = size_t;
public:
    NeuralNetwork(size_type inputs, const std::vector<size_type>& topology, ActivationFunction<scalar> activation = ActivationFunctions<scalar>::relu, scalar learningRate = 0.01f)
        : m_learningRate(learningRate), m_activation(activation)
    {
        //Resize layers to how many layers network will have
        m_neuron_layers.resize(topology.size());
        m_unactivated_neuron_layers.resize(layers_size());
        m_neuron_layer_errors.resize(layers_size());

        m_inputs.resize(inputs);

        for (size_type i = 0; i < topology.size(); ++i)
        {
            m_neuron_layers[i].resize(topology[i]);
            m_unactivated_neuron_layers[i].resize(layer_size(i));
            m_neuron_layer_errors[i].resize(layer_size(i));
        }

        //Initialize weights and resize them according to last layer 
        m_neuron_weights.resize(layers_size());
        srand(time(NULL));
        constexpr scalar weightRandRange = scalar(WEIGHT_RANDOM_RANGE);

        //Initialize input weights
        m_neuron_weights[0].resize(input_layer_size());
        for (size_type j = 0; j < input_layer_size(); ++j)
        {
            m_neuron_weights[0][j].resize(inputs + 1);
            m_neuron_weights[0][j].back() = 0; //Init last weight for each neuron to 0 (because it's a bias)
            for (size_type k = 0; k < inputs; ++k)
            {
                m_neuron_weights[0][j][k] = (scalar(rand()) / RAND_MAX * 2 - 1) * scalar(weightRandRange);
            }
        }

        for (size_type i = 1; i < topology.size(); ++i)
        {
            m_neuron_weights[i].resize(layer_size(i));
            for (size_type j = 0; j < layer_size(i); ++j)
            {
                size_type lastLayerOutputSize = layer_size(i - 1);
                m_neuron_weights[i][j].resize(lastLayerOutputSize + 1);
                m_neuron_weights[i][j].back() = 0;
                for (size_type k = 0; k < lastLayerOutputSize; ++k)
                {
                    m_neuron_weights[i][j][k] = (scalar(rand()) / RAND_MAX * 2 - 1) * scalar(weightRandRange);
                }
            }
        }
    }

    layer forward(const scalar* inputs)
    {
        std::copy(inputs, inputs + input_size(), m_inputs.data());

        for (size_type i = 0; i < input_layer_size(); ++i)
        {
            m_unactivated_neuron_layers[0][i] = std::inner_product(inputs, inputs + input_size(), m_neuron_weights[0][i].data(), m_neuron_weights[0][i][input_size()]);
            m_neuron_layers[0][i] = m_activation.activation(m_unactivated_neuron_layers[0][i]);
        }

        for (size_type i = 1; i < layers_size(); ++i)
        {
            for (size_type j = 0; j < layer_size(i); ++j)
            {
                m_unactivated_neuron_layers[i][j] = std::inner_product(m_neuron_layers[i - 1].begin(), m_neuron_layers[i - 1].end(), m_neuron_weights[i][j].begin(), m_neuron_weights[i][j][m_neuron_layers[i - 1].size()]);
                m_neuron_layers[i][j] = m_activation.activation(m_unactivated_neuron_layers[i][j]);
            }
        }
        return m_neuron_layers.back();
    }
    void backpropagate(const scalar* targets)
    {
        calculateErrors(targets);
        updateWeights();
    }
    void calculateErrors(const scalar* targets)
    {
        for (size_type i = 0; i < output_size(); ++i)
        {
            m_neuron_layer_errors.back()[i] = targets[i] - m_neuron_layers.back()[i];
        }

        for (long i = layers_size() - 2; i >= 0; --i)
        {
            for (size_type j = 0; j < layer_size(i); ++j)
            {
                m_neuron_layer_errors[i][j] = 0;
                for (size_type k = 0; k < layer_size(i + 1); ++k)
                {
                    m_neuron_layer_errors[i][j] += m_neuron_layer_errors[i + 1][k] * m_neuron_weights[i + 1][k][j]; 
                }
            }
        }
    }
    void updateWeights()
    {
        for (size_type j = 0; j < input_layer_size(); ++j)
        {
            for (size_type k = 0; k < input_size(); ++k)
            {
                m_neuron_weights[0][j][k] += m_learningRate * m_neuron_layer_errors[0][j] * m_activation.derivative(m_unactivated_neuron_layers[0][j]) * m_inputs[k];               
            }
        }

        for (size_type i = 1; i < layers_size(); ++i)
        {
            for (size_type j = 0; j < layer_size(i); ++j)
            {
                for (size_type k = 0; k < layer_size(i - 1); ++k)
                {
                    m_neuron_weights[i][j][k] += m_learningRate * m_neuron_layer_errors[i][j] * m_activation.derivative(m_unactivated_neuron_layers[i][j]) * m_neuron_layers[i - 1][k];                 
                }
            }
        }

        //Bias calcs
        for (size_type i = 0; i < layers_size(); ++i)
        {
            for (size_type j = 0; j < layer_size(i); ++j)
            {
                m_neuron_weights[i][j].back() += m_learningRate * m_neuron_layer_errors[i][j] * m_activation.derivative(m_unactivated_neuron_layers[i][j]);
            }
        }
    }
    void train(const batch inputs, const batch targets, const std::string& learningResultsFilePath = "test_results.csv")
    {
        std::ofstream results(learningResultsFilePath);
        results.clear();
        results << "Errors, Iterations" << std::endl;
        for (size_type i = 0; i < inputs.size(); ++i)
        {
            forward(inputs[i].data());
            backpropagate(targets[i].data());
            if (i % 16 == 0)results << mse() << ',' << i << std::endl;
        }
        results.close();
    }

    //I have 3 CSV files in project folder ("test_in.csv", "test_out.csv", "test_results.csv")
    //test_in is converted to neural network's inputs vector
    //test_out is converted to target vector
    //test_results is for debugging, used to plug it to graphing website
    void train(const std::string& filePath)
    {
        batch inputs;
        batch targets;
        readCSV(filePath + "_in.csv", inputs);
        readCSV(filePath + "_out.csv", targets);
        train(inputs, targets, filePath + "_results.csv");
    }

    void readCSV(const std::string& filePath, batch& data)
    {
        data.clear();
        std::ifstream file(filePath);
        file.seekg(0, std::ios::end);
        char* content = new char[file.tellg()];
        file.seekg(0, std::ios::beg);
        
        std::string line, firstLine;

        size_type index = 0;
        size_type iter = 0;

        std::getline(file, firstLine);
        size_t numInputs = std::count(firstLine.begin(), firstLine.end(), ',') + 1;
        file.seekg(0, std::ios::beg);


        while (std::getline(file, line))
        {
            line = std::move(line) + ',';

            data.emplace_back(numInputs);
            for (size_type j = 0; j < numInputs; ++j)
            {
                size_t found = line.find(',', 0);
                if (found == std::string::npos)continue;
                scalar value = scalar(std::stof(line.substr(0, found)));
                line = line.substr(found + 1, line.size());
                data[iter][j] = value;
            }

            memcpy(content + index, line.data(), (line.size() + 1));
            index += line.size() + 1;
            ++iter;
        }
        file.close();
        delete[] content;
    }

public: //<UTILITIES>//
    inline size_type layers_size() const { return m_neuron_layers.size(); }
    inline size_type layer_size(size_type index) const { return m_neuron_layers[index].size(); }
    inline size_type input_size() const { return m_neuron_weights[0][0].size() - 1; }
    inline size_type input_layer_size() const { return layer_size(0); }
    inline size_type output_size() const { return m_neuron_layers.back().size(); }
    inline scalar mse() const 
    { 
        scalar error = 0; 
        for (size_type i = 0; i < output_size(); ++i)
        {
            error += m_neuron_layer_errors.back()[i] * m_neuron_layer_errors.back()[i];
        }
        return std::sqrt(error / output_size());
    }
public: //<DEBUG>//
    friend std::ostream& operator << (std::ostream& os, const NeuralNetwork& net)
    {
        os << "Error: " << net.mse();
        return os;
    }
    void print()
    {
        std::ostream_iterator<scalar> carr(std::cout, " | ");
        std::cout << "Network = \n{" << std::endl;

        for (size_type i = 0; i < m_neuron_layers.size(); ++i)
        {
            std::cout << ("    Layer[" + std::to_string(i) + "] = \n    {") << std::endl;
            for (size_type j = 0; j < m_neuron_layers[i].size(); ++j)
            {
                std::cout << ("        Neuron[" + std::to_string(j) + "] = \n        {") << std::endl;
                std::cout << ("            Weights = [ | ");
                std::copy(m_neuron_weights[i][j].begin(), m_neuron_weights[i][j].end() - 1, carr);
                std::cout << "];" << std::endl;
                std::cout << ("            Bias = " + std::to_string(m_neuron_weights[i][j].back()) + ';') << std::endl;
                std::cout << ("            Unactivated_Output = " + std::to_string(m_unactivated_neuron_layers[i][j]) + ';') << std::endl;
                std::cout << ("            Output = " + std::to_string(m_neuron_layers[i][j]) + ';') << std::endl;
                std::cout << ("            Error = " + std::to_string(m_neuron_layer_errors[i][j]) + ';') << std::endl;
                std::cout << "        };" << std::endl;
            }
            std::cout << "    }" << std::endl;
        }

        std::cout << "\n    Outputs = [ | ";
        std::copy(m_neuron_layers.back().data(), m_neuron_layers.back().data() + output_size(), carr);
        std::cout << "];" << std::endl;

        std::cout << "}" << std::endl;
    }

private:
    layer m_inputs;
    ActivationFunction<scalar> m_activation;
    neuron_layers m_neuron_layers;
    neuron_layers m_unactivated_neuron_layers;
    neuron_layers m_neuron_layer_errors;
    neuron_weights m_neuron_weights;
    scalar m_learningRate;
};
\$\endgroup\$
6
  • \$\begingroup\$ I could have made actuvation function stuff bit cleaner with virtual classes, but that has a performance impact (I am pretty sure). \$\endgroup\$ – NameThatDisplays Jun 15 at 14:03
  • \$\begingroup\$ The use of a pair of function pointers is just fine. A class containing two named virtual functions would be just the same. I suppose this way saves one memory access, since the function objects are stored directly rather than in a vtable. \$\endgroup\$ – JDługosz Jun 15 at 14:53
  • \$\begingroup\$ Is the code working as expected? Asking if something is wrong could lead us to believe the code is broken which would make the question off-topic. \$\endgroup\$ – pacmaninbw Jun 15 at 14:58
  • \$\begingroup\$ yes, it's working well. which is pretty suspicious honestly \$\endgroup\$ – NameThatDisplays Jun 15 at 15:07
  • \$\begingroup\$ "because I like to plug CSV's to a website which converts CSV to Line graph to see how is my network learning". Would you like to share which website that is? \$\endgroup\$ – Jeri Jun 16 at 4:45
6
\$\begingroup\$

#define WEIGHT_RANDOM_RANGE .1
Don't use #define to make your constants. Use constexpr.

In your CSV parser:

 scalar value = scalar(std::stof(line.substr(0, found)));
 line = line.substr(found + 1, line.size());

That's grossly inefficient. The string::substr creates a new string and copies data. Then you shorten the input by using substr again and assigning back to the line, duplicating, re-duplicating, and then destroying the intermediate copy.

Normally you would scan the string without modifying it by using iterators, or a starting index position with the string member functions.

Instead of stof (which, BTW, breaks your abstraction of scalar and assumes that it is in fact float) use from_chars which does not require the input to be copied into its own isolated string first.

line = std::move(line) + ',';
That's an odd way to append a character to a string. What's wrong with push_back?

inline size_type layers_size() const { return m_neuron_layers.size(); }
Good that you use const for accessor functions 😀. But, you don't need to use the inline keyword here. Functions defined inside a class are implicitly inline.

for (size_type i = 0; i < output_size(); ++i)
        {
            error += m_neuron_layer_errors.back()[i] * m_neuron_layer_errors.back()[i];
        }

You're repeatedly calling m_neuron_layer_errors.back(), when it doesn't change within the loop let alone within the expression! You're also subscripting it with the same value more than once.

You seem to be iterating over all the elements in a vector. So don't use a counting loop! Just iterate over the vector directly.

for (const auto x : m_neuron_layer_errors.back())
    error += x*x;

You can apply this idea in many loops in your code.


Your function defined: layer forward(const scalar* inputs)
will return a copy of an entire vector.

You then call it: forward(inputs[i].data()); throwing away the return value. This is pointless and inefficient.

\$\endgroup\$
5
  • \$\begingroup\$ wow, thanks this is very helpful \$\endgroup\$ – NameThatDisplays Jun 15 at 14:52
  • 1
    \$\begingroup\$ x is a float (scalar type), right? So making a reference to it would be slower than copying the value. It's probably going to be held in a vector register ready for use in the math! \$\endgroup\$ – JDługosz Jun 15 at 14:58
  • \$\begingroup\$ (can't upvote) even for your own questions? (It's been a long time since I was new to Stack Overflow) \$\endgroup\$ – JDługosz Jun 15 at 14:59
  • \$\begingroup\$ yup you can't upvote, it requires 15 reputation, also yes x is float completly forgot about that. \$\endgroup\$ – NameThatDisplays Jun 15 at 15:06
  • \$\begingroup\$ I can upvote now thanks to this question, I am deleting old comments \$\endgroup\$ – NameThatDisplays Jun 15 at 15:25
3
\$\begingroup\$
#include <math.h>

Prefer to include <cmath>. That's better, as it declares its identifiers in the std namespace. We'll need to change exp() to std::exp(), but OTOH, std::tanh() becomes correct.

We're missing

  • <cstring>, for std::memcpy()
  • <iterator>, for std::ostream_iterator

std::size_t is misspelt in several places.

template<typename T>
struct ActivationFunctions
{
    inline static const ActivationFunction<T> relu = { [](T x) { return (x > T(0)) ? x : 0; }, [](T x) { return T(x > 0); } };
    inline static const ActivationFunction<T> sigm = { [](T x) { return T(1) / (T(1) + exp(-x)); }, [](T x) { T s = T(1) / (T(1) + exp(-x)); return s * (T(1) - s); } };
    inline static const ActivationFunction<T> tanh = { [](T x) { return std::tanh(x); }, [](T x) { T t = std::tanh(x); return T(1.0f) - t * t; } };
    inline static const ActivationFunction<T> step = { [](T x) { return x <= T(0) ? T(-1) : T(1); }, [](T x) { return 1; } };
    inline static const ActivationFunction<T> none = { [](T x) { return x; }, [](T x) { return 1; } };
};

Constant classes are an anti-pattern, and usually best replaced with functions, perhaps in a namespace. Like this (with newlines - notice how that makes them more readable?):

namespace ActivationFunctions
{
    template<typename T>
    constexpr ActivationFunction<T> relu = {
        [](T x) { return (x > T(0)) ? x : 0; },
        [](T x) { return T(x > 0); }
    };
    template<typename T>
    constexpr ActivationFunction<T> sigm = {
        [](T x) { return T(1) / (T(1) + std::exp(-x)); },
        [](T x) { T s = T(1) / (T(1) + std::exp(-x)); return s * (T(1) - s); }
    };
    template<typename T>
    constexpr ActivationFunction<T> tanh = {
        [](T x) { return std::tanh(x); },
        [](T x) { T t = std::tanh(x); return T(1.0f) - t * t; }
    };
    template<typename T>
    constexpr ActivationFunction<T> step = {
        [](T x) { return x <= T(0) ? T(-1) : T(1); },
        [](T x) { return 1; }
    };
    template<typename T>
    constexpr ActivationFunction<T> none = {
        [](T x) { return x; },
        [](T x) { return 1; }
    };
}

Obviously, that changes how we identify them:

NeuralNetwork(size_type inputs, const std::vector<size_type>& topology,
              ActivationFunction<scalar> activation = ActivationFunctions::relu<scalar>,
              scalar learningRate = 0.01f)

I think we might do better if we help the compiler to inline these functions, by making them part of the NeuralNetwork<T> class. We can do that by inheriting them as a base class (using the Curiously Recurring Template Pattern):

namespace ActivationFunctions
{
    template<typename T>
    struct relu {
        static constexpr auto activation(T x) { return (x > T(0)) ? x : T(0); }
        static constexpr auto derivative(T x) { return T(x > 0); }
    };
    ⋮
}
template<typename T = float,
         template<typename> class AF = ActivationFunctions::relu>
class NeuralNetwork : AF<T>

We then remove the m_activation member, and just call the functions through the (implicit) this pointer. E.g. m_activation.activation(…) becomes simply activation(…).

Obviously this route makes it more difficult to select an activation function at runtime; without the calling code, it's impossible to say whether that's an obstacle to be overcome.

The print() function unnecessarily flushes output (with std::endl) much more than necessary. I would replace all of those with ordinary non-flushing newline, '\n'.

\$\endgroup\$
8
  • \$\begingroup\$ I used to use namespaces for every function instead of static classes or such, when I was newcomer from java, is it bad to do so, if so what should I use? \$\endgroup\$ – NameThatDisplays Jun 15 at 15:33
  • \$\begingroup\$ thanks for suggestions. \$\endgroup\$ – NameThatDisplays Jun 15 at 15:38
  • \$\begingroup\$ I dont think std::function<T> can be constexpr, can it? \$\endgroup\$ – NameThatDisplays Jun 15 at 15:41
  • \$\begingroup\$ GCC didn't complain - but I never instantiated the NeuralNetwork, because there's no main(). \$\endgroup\$ – Toby Speight Jun 15 at 15:54
  • \$\begingroup\$ Having instantiated, I now get the error for the constexpr. But these are pure functions, so perhaps std::function is overkill anyway? \$\endgroup\$ – Toby Speight Jun 15 at 16:14
2
\$\begingroup\$

Loops

You criminally underuse the range-based for. JDlogusz already mentioned this, but it cannot be said enough. Avoid index variables whenever possible.

Memory management

char* content = new char[file.tellg()];
//...
delete[] content;

new and delete are code smells. The only acceptable times for manual memory management in C++ are when you are writing your own smart pointers or allocators. Otherwise, use the STL smart pointers. For example:

//Avoid new entirely
auto content{std::make_unique<char[]>(file.tellg())};
//Use new, but let compiler handle lifetime
//(dangerous! --- see https://stackoverflow.com/a/22571331/4926165)
std::unique_ptr<char[]> content(new char[file.tellg()]);
//For a raw pointer:
char *ptr{content.get()};

//Container classes also function as smart pointers
std::vector<char[]> content(file.tellg());
char *ptr{content.data()};
std::string content('\0xF', file.tellg());
char *ptr{content.c_str()};

Randomness

rand is very rarely the correct randomness engine for your needs. Most implementations are very poor and it is hard to turn rand's output into useful values; see STL's presentation. Consider using C++'s pseudo-random number libraries instead.

Functors

ActivationFunction<scalar> m_activation;

std::function has very high overhead. Are you really likely to be changing which activation function you use based on user input? If not, you can determine it at compile time. Consider passing a Functor object as type parameter to your class, like so:

struct relu
{
    template<typename T>
    auto operator()(T input) const noexcept {return std::max<T>(input, 0);}
    struct deriv_t {
        template<typename T>
        auto operator()(T input) const noexcept {return T(input > 0);}
    } derivative;
};
template<typename Scalar = float, typename Activation = relu>
class NeuralNetwork
{
    //...
    m_neuron_layers[0][i] = activation(m_unactivated_neuron_layers[0][i]);
    //...
    m_neuron_weights[0][j][k] += m_learningRate * m_neuron_layer_errors[0][j] * m_activation.derivative(m_unactivated_neuron_layers[0][j]) * m_inputs[k];
    //...
    Activation m_activation;
}

Moreover, why are you separating out the function and its derivative? C++ supports operator overloading. Consider using automatic differentiation instead.

Separating out test code

void train(const batch inputs, const batch targets, const std::string& learningResultsFilePath = "test_results.csv");

Why are you hardcoding that output should go to a file? Instead, consider taking an output stream or even an output iterator (courtesy of ostream_iterator).

    void NeuralNetwork::train(const std::string& filePath);
    void NeuralNetwork::readCSV(const std::string& filePath, batch& data);

Why are these in NeuralNetwork? Not everything has to be in a class; not everything has to be in the same class. Consider putting NeuralNetwork inside a namespace (say, nn), and then these functions inside nn or nn::Test.

public: //<DEBUG>//
    friend std::ostream& operator << (std::ostream& os, const NeuralNetwork& net);
    void print();

If these functions are debug-only, why aren't they only compiled in debug mode? For example (MSVS):

#ifndef NDEBUG
public: //<DEBUG>//
    friend std::ostream& operator<<(std::ostream& os, const NeuralNetwork& net);
    void print();
#endif

Parsing

for (size_type j = 0; j < numInputs; ++j)
{
    size_t found = line.find(',', 0);
    if (found == std::string::npos)continue;
    scalar value = scalar(std::stof(line.substr(0, found)));
    line = line.substr(found + 1, line.size());
    data[iter][j] = value;
}

This parsing code is brittle. Consider using a state machine or stringstream instead:

{
    std::istringstream line_{line};
    scalar value;
    char delim;
    while(line_ >> value >> delim)
    {
        assert(','==delim);
        data[iter].emplace_back(std::move(value));
    }
}

(Yes, the std::move is probably overkill, but better safe than sorry….)

Also, you again should consider a design that accepts arbitrary stream/iterator inputs. That seeking and telling at the beginning of readCSV is a major code smell to me.

Function Signatures

void readCSV(const std::string& filePath, batch& data);

What do you have against return values?

batch readCSV(const std::string &filePath);

None of your private functions return a value. That suggests to me that they probably keep too much information in the state of your neural network. (It should end up there eventually, don't get me wrong! Just I would expect calling code to do the assignments:

m_neuron_layers = forward(inputs.data())

Of course, if you aren't changing all the values in m_neuron_layers or want to avoid memory allocations, then maybe this makes sense.)

\$\endgroup\$
2
  • \$\begingroup\$ ReadCSV is a code smell I know, I got rid of it, because I don't need to actually write everything to file, not sure what made me think that, but good thing I left it here, now am getting lots of tips I did not know \$\endgroup\$ – NameThatDisplays Jun 16 at 11:23
  • \$\begingroup\$ Rand() should be fine tho, I don't think it will cause any problems in my case, thanks for the detailed review. \$\endgroup\$ – NameThatDisplays Jun 16 at 11:28

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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