9
\$\begingroup\$

I wrote a simple softmax classifier to classify MNIST digit handwriting data set. Feel free to comment anything!

#include <algorithm>
#include <bit>
#include <cassert>
#include <chrono>
#include <cmath>
#include <cstdint>
#include <execution>
#include <filesystem>
#include <fstream>
#include <iostream>
#include <iterator>
#include <numeric>
#include <memory>
#include <random>
#include <string>
#include <type_traits>
#include <vector>

struct MNISTObject {
    std::vector<double> image;
    std::uint8_t label = -1;
    static std::uint32_t rows;
    static std::uint32_t cols;
    static std::uint8_t labels;
};

std::uint32_t MNISTObject::rows = 28;
std::uint32_t MNISTObject::cols = 28;
std::uint8_t MNISTObject::labels = 10;

constexpr std::uint32_t image_code = 2051;
constexpr std::uint32_t label_code = 2049;

void convertBigToLittleEndianIfNecessary(uint32_t& code) {
    if constexpr (std::endian::native == std::endian::little) {
        code = ((code & 0xFF000000) >> 24) |
               ((code & 0x00FF0000) >> 8) |
               ((code & 0x0000FF00) >> 8) |
               ((code & 0x000000FF) >> 24);
    }
}

void read_uint32(std::istream& is, std::uint32_t& code) {
    if (!is.read(reinterpret_cast<char*>(&code), sizeof code)) {
        std::cerr << "Cannot read uint32\n";
        return;
    }
    convertBigToLittleEndianIfNecessary(code);
}

void fillDataset(std::istream& images, std::istream& labels, std::vector<MNISTObject>& data_set) {
    std::uint32_t code;
    read_uint32(images, code);
    if (code != image_code) {
        std::cerr << "Wrong image code : " << code << '\n';
        return;
    }
    read_uint32(labels, code);
    if (code != label_code) {
        std::cerr << "Wrong label code : " << code << '\n';
        return;
    }

    std::uint32_t cnt_images, cnt_labels;
    read_uint32(images, cnt_images);
    read_uint32(labels, cnt_labels);
    if (cnt_images != cnt_labels) {
        std::cerr << "Image/label counts do not match\n";
        return;
    } else {
        std::cout << cnt_images << " images\n";
    }

    std::uint32_t rows, cols;

    read_uint32(images, rows);
    read_uint32(images, cols);
    if (rows != MNISTObject::rows || cols != MNISTObject::cols) {
        std::cerr << "Wrong rows and cols: " << rows << ' ' << cols << '\n';
        return;
    }
    std::cout << rows << " rows " << cols << " cols\n";

    std::uint32_t img_size = rows * cols;

    data_set.reserve(cnt_images);
    for (std::size_t i = 0; i < cnt_images; ++i) {
        MNISTObject obj;
        obj.image.reserve(img_size);
        std::uint8_t pixel;
        for (std::size_t p = 0; p < img_size; ++p) {
            if (!images.read(reinterpret_cast<char*>(&pixel), sizeof pixel)) {
                std::cerr << "Pixel read fail\n";
                return;
            }
            double pixel_val = static_cast<double>(pixel) / 255.0;
            obj.image.push_back(pixel_val);
        }
        std::uint8_t label;
        if (!labels.read(reinterpret_cast<char*>(&label), sizeof label)) {
            std::cerr << "Label read fail\n";
            return;
        }
        if (label >= MNISTObject::labels) {
            std::cerr << "Wrong label : " << static_cast<std::uint32_t>(label) << '\n';
            return;
        }
        obj.label = label;
        data_set.push_back(obj);
    }
    std::cout << cnt_images << " images read success\n";
}

std::pair<std::vector<MNISTObject>, std::vector<MNISTObject>> constructDataSets() {
    std::filesystem::path train_images_path = "train-images.idx3-ubyte";
    std::filesystem::path train_labels_path = "train-labels.idx1-ubyte";
    std::ifstream train_images_ifs {train_images_path, std::ios::binary};
    if (!train_images_ifs) {
        std::cerr << "Cannot open input file " << train_images_path;
    }
    std::ifstream train_labels_ifs {train_labels_path, std::ios::binary};
    if (!train_labels_ifs) {
        std::cerr << "Cannot open input file " << train_labels_path;
    }

    std::vector<MNISTObject> train_set;
    fillDataset(train_images_ifs, train_labels_ifs, train_set);

    std::filesystem::path test_images_path = "t10k-images.idx3-ubyte";
    std::filesystem::path test_labels_path = "t10k-labels.idx1-ubyte";
    std::ifstream test_images_ifs {test_images_path, std::ios::binary};
    if (!test_images_ifs) {
        std::cerr << "Cannot open input file " << test_images_path;
    }
    std::ifstream test_labels_ifs {test_labels_path, std::ios::binary};
    if (!test_labels_ifs) {
        std::cerr << "Cannot open input file " << test_labels_path;
    }

    std::vector<MNISTObject> test_set;
    fillDataset(test_images_ifs, test_labels_ifs, test_set);

    return {train_set, test_set};
}

template <typename T>
struct Matrix {
    static_assert(std::is_scalar_v<T>);
    const int R;
    const int C;
    std::unique_ptr<T[]> data;

    Matrix(int R, int C) : R {R}, C {C}, data(new T[R * C]) {
        assert(R > 0 && C > 0);
    }

    T& operator()(int r, int c) {
        assert(0 <= r && r < R && 0 <= c && c < C);
        return data[r * C + c];
    }

    const T& operator()(int r, int c) const {
        assert(0 <= r && r < R && 0 <= c && c < C);
        return data[r * C + c];
    }
};

std::mt19937 gen(std::random_device{}());

std::vector<double> computeProb(const Matrix<double>& W, const MNISTObject& mnist_sample) {
    assert(W.R == MNISTObject::labels && W.C == MNISTObject::rows * MNISTObject::cols);
    std::vector<double> probs;
    probs.reserve(MNISTObject::labels);
    double sum_probs = 0.0;
    for (int l = 0; l < MNISTObject::labels; l++) {
        auto inner_prod = std::transform_reduce(std::execution::par_unseq,
                                                &W.data[l * W.C], &W.data[(l + 1) * W.C],
                                                mnist_sample.image.begin(), 0.0);
        auto prob = std::exp(inner_prod);
        probs.push_back(prob);
        sum_probs += prob;
    }
    std::for_each(std::execution::par_unseq, probs.begin(), probs.end(), [&sum_probs](auto& p){p /= sum_probs;});
    auto real_sum = std::reduce(std::execution::par_unseq, probs.begin(), probs.end(), 0.0);
    if (std::fabs(real_sum - 1.0) >= 1e-5) {
        std::cerr << "Probability sum failed in softmax: " << real_sum << '\n';
    }
    return probs;
}

constexpr int num_epochs = 100;

double testSoftmaxClassifier(const Matrix<double>& W, const std::vector<MNISTObject>& test_set) {
    int correct = 0;
    int incorrect = 0;
    for (const auto& test_sample : test_set) {
        auto probs = computeProb(W, test_sample);
        auto predict = std::distance(probs.begin(), std::ranges::max_element(probs));
        if (predict == test_sample.label) {
            correct++;
        } else {
            incorrect++;
        }
    }
    return correct / (correct + incorrect * 1.0);
}

Matrix<double> trainSoftmaxClassifier(const std::vector<MNISTObject>& train_set,
                                      const std::vector<MNISTObject>& test_set,
                                      double lr, double weight_decay) {
    std::uniform_real_distribution<> weight_dist(-1.0, 1.0);
    const int k = MNISTObject::labels;
    const int sz = MNISTObject::rows * MNISTObject::cols;
    Matrix<double> W (k, sz);
    for (int i = 0; i < k * sz; ++i) {
        W.data[i] = weight_dist(gen);
    }

    for (int epoch = 0; epoch < num_epochs; epoch++) {
        auto t1 = std::chrono::high_resolution_clock::now();
        for (const auto& train_sample : train_set) {
            auto probs = computeProb(W, train_sample);
            auto correct = train_sample.label;
            for (int l = 0; l < k; ++l) {
                for (int p = 0; p < sz; ++p) {
                    W(l, p) = W(l, p) * (1.0 - lr * weight_decay)
                            - lr * train_sample.image[p] * probs[l];
                }
                if (l == correct) {
                    for (int p = 0; p < sz; ++p) {
                        W(l, p) += lr * train_sample.image[p];
                    }
                }
            }
        }
        auto t2 = std::chrono::high_resolution_clock::now();
        auto dt = std::chrono::duration_cast<std::chrono::milliseconds>(t2 - t1);
        std::cout << "epoch " << epoch << " finished in " << dt.count() << "ms\n";

        auto accu = testSoftmaxClassifier(W, test_set);
        std::cout << "Accuracy : " << accu << '\n';
    }

    return W;
}

int main() {
    auto [train_set, test_set] = constructDataSets();

    constexpr double lr = 0.0005;
    constexpr double weight_decay = 0;

    auto W = trainSoftmaxClassifier(train_set, test_set, lr, weight_decay);

}

You can download the dataset here: http://yann.lecun.com/exdb/mnist/

Result in my machine:

60000 images
28 rows 28 cols
60000 images read success
10000 images
28 rows 28 cols
10000 images read success
epoch 0 finished in 1132ms
Accuracy : 0.7536
epoch 1 finished in 1142ms
Accuracy : 0.8176
epoch 2 finished in 987ms
Accuracy : 0.8424
epoch 3 finished in 1094ms
Accuracy : 0.8574
epoch 4 finished in 1038ms
Accuracy : 0.8662
epoch 5 finished in 1191ms
Accuracy : 0.8724
epoch 6 finished in 1087ms
Accuracy : 0.8789
epoch 7 finished in 1020ms
Accuracy : 0.8838
epoch 8 finished in 1170ms
Accuracy : 0.8867
epoch 9 finished in 1150ms
Accuracy : 0.889
epoch 10 finished in 791ms
Accuracy : 0.8908
epoch 11 finished in 815ms
Accuracy : 0.8919
epoch 12 finished in 809ms
Accuracy : 0.8929
epoch 13 finished in 792ms
Accuracy : 0.8934
epoch 14 finished in 798ms
Accuracy : 0.894
epoch 15 finished in 816ms
Accuracy : 0.8959
epoch 16 finished in 826ms
Accuracy : 0.8965
epoch 17 finished in 990ms
Accuracy : 0.8978
epoch 18 finished in 804ms
Accuracy : 0.8983
epoch 19 finished in 907ms
Accuracy : 0.8995
epoch 20 finished in 800ms
Accuracy : 0.9002
...
\$\endgroup\$

4 Answers 4

16
\$\begingroup\$

In addition to Toby Speight's remarks, I would add:

Use constexpr for all constants

I see you already used constexpr for image_code and label_code, but you didn't use it for the static member variables rows, cols and labels of MNISTObject. But those look like they are constants as well. You can just write:

struct MNISTObject {
    ...
    static constexpr std::uint32_t rows = 28;
    static constexpr std::uint32_t cols = 28;
    static constexpr std::uint8_t labels = 10;
};

Make variables and functions static where appropriate

Variables and functions that are only used within the same translation unit should be made static. This might help the compiler produce more efficient code, and avoids potential symbol collision at link time.

Prefer returning values from functions

I see a lot of functions that return void, but write their result via a parameter passed by reference. I would avoid this pattern and instead use return to return the result. First, passing in a reference is less efficient if the compiler cannot inline the function. But more importantly, it just makes it harder to write code. Consider:

std::uint32_t code;
read_uint32(images, code);
if (code != image_code) {...}

If you instead let read_uint32() return the value it read, you can write the above snippet of code as:

if (read_uint32(images) != image_code) {...}

Another benefit is that it will now force you to return something, even if an error occured. Because in this code:

std::uint32_t code;
read_uint32(images, code);

The value of code is left uninitialized if there is a read error, but your program continues to go on. There is a tiny chance that the value 2051 would have been in the memory reserved for code, so your program might incorrectly go on assuming the right image code was read. I would follow Toby's advice here and throw an exception (preferably a std::runtime_error or something derived from it):

uint32_t read_uint32(std::istream& is) {
    if (uint32_t code; is.read(reinterpret_cast<char*>(&code), sizeof code)) {
        return convertBigToLittleEndianIfNecessary(code);
    } else {
        throw std::runtime_error("Read error");
    }
}

Also note that you should also return for large objects, which is still as efficient as passing it via a reference parameter thanks to return value optimization. So fillDataSet should just return the dataset (and have its name changed to readDataSet() to reflect that).

Optimize reading from files

You are reading the image data from the input file byte by byte. This is quite inefficient. I suggest you read the whole image in one go instead:

auto pixels = std::make_unique_for_ovewrite<std::uint8_t[]>(img_size);
if (!images.read(reinterpret_cast<char*>(pixels.get()), img_size)) {
    throw std::runtime_error("Read error");
}

std::transform(pixels.get(), pixels.get() + img_size, std::back_inserter(obj.image),
               [](std::uint8_t pixel){ return pixel / 255.0; });
\$\endgroup\$
5
  • \$\begingroup\$ Upvoted. Isn't make_unique more preferable than make_unique_for_overwrite? There will be unnecessary zero-fill cost. \$\endgroup\$
    – frozenca
    Apr 25, 2021 at 19:18
  • \$\begingroup\$ std::make_unique_for_overwrite is the one that avoids "zero-fill cost" if possible; it uses default-initialization for the storage, whereas std::make_unique uses value-initialization. The name is also a hint: it should be used when you are going to overwrite the values anyway, and that's exactly what we're doing when reading the image data into pixels. \$\endgroup\$
    – G. Sliepen
    Apr 25, 2021 at 21:58
  • \$\begingroup\$ I personally think that anonymous namespaces look somewhat nicer than static functions, but functionally they are the same. \$\endgroup\$ Apr 26, 2021 at 18:52
  • \$\begingroup\$ You are worried about the overhead of zero-initialising a memory block when reading a file? Lol. Any gains resulting from would be impossible to measure because they are swamped by the time taken to read the file. \$\endgroup\$
    – Sjoerd
    Apr 27, 2021 at 8:35
  • \$\begingroup\$ @Sjoerd A std::istream doesn't have to be a file, and even if it is, the file might already be cached in memory. \$\endgroup\$
    – G. Sliepen
    Apr 27, 2021 at 10:23
10
\$\begingroup\$

That's a very big list of includes! It might be a sign that you have functionality that should be split out to separate source files (e.g. filesystem read/write away from the core computation). Keeping the standard library includes alphabetical is a good choice - it allows very easy checking of whether a needed header is already included.

This looks broken, and suggests you're lacking a good unit test:

void convertBigToLittleEndianIfNecessary(uint32_t& code) {
    if constexpr (std::endian::native == std::endian::little) {
        code = ((code & 0xFF000000) >> 24) |
               ((code & 0x00FF0000) >> 8) |
               ((code & 0x0000FF00) >> 8) |
               ((code & 0x000000FF) >> 24);
    }
}

The last subexpression will always be zero - I think that the last two subexpressions should have << where you've written >>.

The error reporting is poor - a lot of void-returning functions print user warnings to std::cerr (good) but have no way to signal to their callers that they will be working with invalid data (bad). It would be more useful to throw an exception than to simply return. (Again, more unit-testing would be good - I usually start new code by creating a test with invalid input as my very first step.)

A couple of minor niggles with the matrix class:

  • Is there a good reason to use (signed) int for the dimensions?
  • I'd prefer to see std::make_unique<T[]>(R*C) than the bare new.
  • I don't see why it would only work for scalar T. It's generally better to constrain using Concepts where possible (e.g. template<std::semiregular T> struct Matrix).

I would write correct++, incorrect++ and epoch++ using prefix operator, as that's a good general habit (use postfix operator only when we actually need the original value). The code is inconsistent, since we have ++i and ++p elsewhere.

\$\endgroup\$
2
  • \$\begingroup\$ Good catch for endian conversion, thanks! I always prefer signed integers for dimensions, because it helps to catch naughty constructs like Matrix<double> M(-1, -1). I agree all the other things you've pointed out. \$\endgroup\$
    – frozenca
    Apr 25, 2021 at 10:01
  • 3
    \$\begingroup\$ Good compiler warnings (-Wconversion for GCC, I think) will alert you to such misuses, unless you accept signed values. I prefer to be notified at compile time than at run time (and I use -Werror to ensure the issues are addressed). It also reduces the number of unit tests to write and maintain. \$\endgroup\$ Apr 25, 2021 at 19:24
8
\$\begingroup\$

Toby Speight and G. Sliepen gave excellent feedbacks from the programmer's perspective;

A friend of mine gave some feedbacks from the machine learning researcher's perspective, as follows:

  • User should be able to specify hyperparameters (lr, weight_decay, etc), or the code should include hyperparameter tuning

  • Weight matrix initialization is wrong. It should use a Gaussian distribution with mean 0, stdev sqrt(2/(#fanin + #fanout))

  • Training should stop early if converged enough.

  • This softmax implementation is vulnerable to overflow/underflow. Use logsumexp trick instead.

  • It'd be good to include an option to specify batch size and work with it

  • double is overkill for such simple jobs like MNIST: use float32 instead

UPDATE: Addressed most of feedbacks.

#include <algorithm>
#include <bit>
#include <cassert>
#include <chrono>
#include <concepts>
#include <cmath>
#include <cstdint>
#include <execution>
#include <filesystem>
#include <fstream>
#include <iostream>
#include <iterator>
#include <numeric>
#include <memory>
#include <random>
#include <queue>
#include <stdexcept>
#include <string>
#include <vector>

struct MNISTObject {
    std::vector<float> image;
    std::uint8_t label = -1;
    static constexpr std::uint32_t rows = 28;
    static constexpr std::uint32_t cols = 28;
    static constexpr std::uint8_t labels = 10;
};

constexpr std::uint32_t image_code = 2051;
constexpr std::uint32_t label_code = 2049;

std::uint32_t convertBigToLittleEndianIfNecessary(std::uint32_t code) {
    if constexpr (std::endian::native == std::endian::little) {
        code = ((code & 0xFF000000) >> 24) |
               ((code & 0x00FF0000) >> 8) |
               ((code & 0x0000FF00) << 8) |
               ((code & 0x000000FF) << 24);
    }
    return code;
}

std::uint32_t read_uint32(std::istream& is) {
    if (std::uint32_t code; is.read(reinterpret_cast<char*>(&code), sizeof code)) {
        return convertBigToLittleEndianIfNecessary(code);
    } else {
        throw std::runtime_error("Couldn't read uint32\n");
    }
}

std::vector<MNISTObject> readDataset(std::istream& images, std::istream& labels) {
    auto code = read_uint32(images);
    if (code != image_code) {
        std::cerr << code << " : ";
        throw std::runtime_error("Wrong image code\n");
    }
    code = read_uint32(labels);
    if (code != label_code) {
        std::cerr << code << " : ";
        throw std::runtime_error("Wrong label code\n");
    }

    auto cnt_images = read_uint32(images);
    auto cnt_labels = read_uint32(labels);
    if (cnt_images != cnt_labels) {
        std::cerr << cnt_images << ' ' << cnt_labels << " : ";
        throw std::runtime_error("Image/label counts do not match\n");
    } else {
        std::cout << cnt_images << " images\n";
    }

    auto rows = read_uint32(images);
    auto cols = read_uint32(images);
    if (rows != MNISTObject::rows || cols != MNISTObject::cols) {
        std::cerr << rows << ' ' << cols << " : ";
        throw std::runtime_error("Wrong rows and cols\n");
    }
    std::cout << rows << " rows " << cols << " cols\n";

    std::uint32_t img_size = rows * cols;

    std::vector<MNISTObject> data_set;
    data_set.reserve(cnt_images);
    auto pixels = std::make_unique<std::uint8_t[]>(img_size);
    for (std::size_t i = 0; i < cnt_images; ++i) {
        MNISTObject obj;
        obj.image.reserve(img_size);
        if (!images.read(reinterpret_cast<char*>(pixels.get()), img_size)) {
            throw std::runtime_error("Image read error\n");
        }

        std::transform(pixels.get(), pixels.get() + img_size, std::back_inserter(obj.image),
                       [](std::uint8_t pixel){ return static_cast<float>(pixel) / 255.0f; });
        std::uint8_t label = 0;
        if (!labels.read(reinterpret_cast<char*>(&label), sizeof label)) {
            throw std::runtime_error("Label read error\n");
        }
        if (label >= MNISTObject::labels) {
            std::cerr << static_cast<std::uint32_t>(label) << " : ";
            throw std::runtime_error("Wrong label\n");
        }
        obj.label = label;
        data_set.push_back(obj);
    }
    std::cout << cnt_images << " images read success\n";
    return data_set;
}

std::pair<std::vector<MNISTObject>, std::vector<MNISTObject>> constructDataSets() {
    std::filesystem::path train_images_path = "train-images.idx3-ubyte";
    std::filesystem::path train_labels_path = "train-labels.idx1-ubyte";
    std::ifstream train_images_ifs {train_images_path, std::ios::binary};
    if (!train_images_ifs) {
        throw std::runtime_error("Cannot open train images file\n");
    }
    std::ifstream train_labels_ifs {train_labels_path, std::ios::binary};
    if (!train_labels_ifs) {
        throw std::runtime_error("Cannot open train labels file\n");
    }

    auto train_set = readDataset(train_images_ifs, train_labels_ifs);

    std::filesystem::path test_images_path = "t10k-images.idx3-ubyte";
    std::filesystem::path test_labels_path = "t10k-labels.idx1-ubyte";
    std::ifstream test_images_ifs {test_images_path, std::ios::binary};
    if (!test_images_ifs) {
        throw std::runtime_error("Cannot open test images file\n");
    }
    std::ifstream test_labels_ifs {test_labels_path, std::ios::binary};
    if (!test_labels_ifs) {
        throw std::runtime_error("Cannot open test labels file\n");
    }

    auto test_set = readDataset(test_images_ifs, test_labels_ifs);

    return {train_set, test_set};
}

template <std::regular T>
struct Matrix {
    const int R;
    const int C;
    std::unique_ptr<T[]> data;

    Matrix(int R, int C) : R {R}, C {C}, data(std::make_unique<T[]>(R * C)) {
        assert(R > 0 && C > 0);
    }

    T& operator()(int r, int c) {
        assert(0 <= r && r < R && 0 <= c && c < C);
        return data[r * C + c];
    }

    const T& operator()(int r, int c) const {
        assert(0 <= r && r < R && 0 <= c && c < C);
        return data[r * C + c];
    }
};

std::mt19937 gen(std::random_device{}());

std::vector<float> computeProb(const Matrix<float>& W, const MNISTObject& mnist_sample) {
    assert(W.R == MNISTObject::labels && W.C == MNISTObject::rows * MNISTObject::cols);
    std::vector<float> probs;
    probs.reserve(MNISTObject::labels);
    for (int l = 0; l < MNISTObject::labels; l++) {
        auto prob = std::transform_reduce(std::execution::par_unseq,
                                                &W.data[l * W.C], &W.data[(l + 1) * W.C],
                                                mnist_sample.image.begin(), 0.0f);
        probs.push_back(prob);
    }
    auto max_prob = *std::ranges::max_element(probs);
    std::for_each(std::execution::par_unseq, probs.begin(), probs.end(), [&max_prob](auto& p){p = std::exp(p - max_prob);});
    auto sum_probs = std::reduce(std::execution::par_unseq, probs.begin(), probs.end(), 0.0f);
    std::for_each(std::execution::par_unseq, probs.begin(), probs.end(), [&sum_probs](auto& p){p /= sum_probs;});
    return probs;
}

float testSoftmaxClassifier(const Matrix<float>& W, const std::vector<MNISTObject>& test_set) {
    int correct = 0;
    int incorrect = 0;
    for (const auto& test_sample : test_set) {
        auto probs = computeProb(W, test_sample);
        auto predict = std::distance(probs.begin(), std::ranges::max_element(probs));
        if (predict == test_sample.label) {
            correct++;
        } else {
            incorrect++;
        }
    }
    return static_cast<float>(correct) / static_cast<float>(correct + incorrect);
}

constexpr int num_epochs = 300;

Matrix<float> trainSoftmaxClassifier(const std::vector<MNISTObject>& train_set,
                                      const std::vector<MNISTObject>& test_set,
                                      float lr, float weight_decay) {
    const int k = MNISTObject::labels;
    const int sz = MNISTObject::rows * MNISTObject::cols;
    std::normal_distribution<float> weight_dist(0, std::sqrt(2.0f / (k + sz * 1.0f)));
    Matrix<float> W (k, sz);
    for (int i = 0; i < k * sz; ++i) {
        W.data[i] = weight_dist(gen);
    }

    std::deque<float> prev_accu;
    constexpr int accu_window_size = 5;

    for (int epoch = 0; epoch < num_epochs; epoch++) {
        auto t1 = std::chrono::high_resolution_clock::now();
        for (const auto& train_sample : train_set) {
            auto probs = computeProb(W, train_sample);
            auto correct = train_sample.label;
            for (int l = 0; l < k; ++l) {
                for (int p = 0; p < sz; ++p) {
                    W(l, p) = W(l, p) * (1.0f - lr * weight_decay)
                              - lr * train_sample.image[p] * probs[l];
                }
                if (l == correct) {
                    for (int p = 0; p < sz; ++p) {
                        W(l, p) += lr * train_sample.image[p];
                    }
                }
            }
        }
        auto t2 = std::chrono::high_resolution_clock::now();
        auto dt = std::chrono::duration_cast<std::chrono::milliseconds>(t2 - t1);
        std::cout << "epoch " << epoch << " finished in " << dt.count() << "ms\n";

        auto accu = testSoftmaxClassifier(W, test_set);
        std::cout << "Accuracy : " << accu << '\n';

        prev_accu.push_back(accu);
        if (prev_accu.size() > accu_window_size) {
            prev_accu.pop_front();
        }
        if (prev_accu.size() == accu_window_size &&
        *std::ranges::max_element(prev_accu) - *std::ranges::min_element(prev_accu) < 3e-4) {
            std::cout << "Converged enough, stopping the training\n";
            break;
        }
    }

    return W;
}

int main() {
    auto [train_set, test_set] = constructDataSets();

    constexpr float lr = 0.0005;
    constexpr float weight_decay = 0;

    auto W = trainSoftmaxClassifier(train_set, test_set, lr, weight_decay);

}

Result:

60000 images
28 rows 28 cols
60000 images read success
10000 images
28 rows 28 cols
10000 images read success
epoch 0 finished in 774ms
Accuracy : 0.8874
epoch 1 finished in 743ms
Accuracy : 0.9004
epoch 2 finished in 847ms
Accuracy : 0.9045
epoch 3 finished in 872ms
Accuracy : 0.9083
epoch 4 finished in 871ms
Accuracy : 0.9109
epoch 5 finished in 849ms
Accuracy : 0.9124
...
epoch 45 finished in 629ms
Accuracy : 0.9226
epoch 46 finished in 606ms
Accuracy : 0.9227
Converged enough, stopping the training

Better than before! Machine learning is fun.

\$\endgroup\$
1
  • 2
    \$\begingroup\$ Thanks for adding this self-answer. It fills in aspects I'm not qualified to comment on and may be educational if I ever have to venture into this territory. \$\endgroup\$ Apr 25, 2021 at 19:20
5
\$\begingroup\$

function should return values

Others have mentioned "return things rather than using output parameters" but here is an elaborated example.

void read_uint32(std::istream& is, std::uint32_t& code)

In this case, it's providing a primitive type so you don't even have the idea of avoiding large return values, so what's up with that? It's not returning an error code to indicate an optional result either, so why would you not just naturally write it as

std::uint32_t read_uint32(std::istream& is)

?

Look at how you need to call it:

std::uint32_t cnt_images, cnt_labels;
read_uint32(images, cnt_images);
read_uint32(labels, cnt_labels);

std::uint32_t rows, cols;
read_uint32(images, rows);
read_uint32(images, cols);

You should initialize variables when defining them and this out-parameter thing gets in the way of that. Having proper initialization also means that you can make them const:

const auto rows = read_uint32(images);
const auto cols = read_uint32(images);

includes?

Your long list of #include directives has some things I don't see being used, and in fact I find are rarely needed, like <iterator>. Did you paste in a long commonly needed list when you started your file, instead of just naming what you actually needed? I'll usually comment on the #include line why I need it, if it's a specific feature that's not pervasive in the file.

\$\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.