0

I tried to make the Matrix header file. I also gave it a go at representing the matrix with a single array. Every method I wrote worked just fine. On the contrary, I couldn't figure out the problem with the matrix multiplication. It seems fine though. I have stripped some code to make it small. any help is appreciated.

#ifndef MATRIX_HPP
#define MATRIX_HPP

#include <iostream>
#include <random>
#include <chrono>
#include <ctime>
#include <iomanip>
using std::cout;
using std::endl;
using std::ios;

static std::ostream & pretty_print(std::ostream & output) {
    output.setf(ios::showpoint);
    output.setf(ios::showpos);
    output.width(6);
    output.precision(2);

    return output;
}

class Matrix {
public:
    // Constructor functions
    Matrix();
    Matrix(size_t r, size_t c, double v = 0);
    Matrix(size_t r, size_t c, double *array);
    Matrix(const Matrix &mat);

    // helping functions
    void randomize(double a = -1, double b = 1);
    void addMat(const Matrix &mat);
    void subMat(const Matrix &mat);
    void multiply_matrix(const Matrix &mat);
    Matrix transpose();
    double* toArray();
    void display();

    // Static functions
    static Matrix matMul(const Matrix &mat1, const Matrix &mat2);
    static Matrix transpose(const Matrix &mat);
    static Matrix fromArray(double *arr, size_t size);

    // Overloaded operator functions and some friend functions
    /*friend ostream& operator<<(ostream &dout, Matrix &mat);
    friend istream& operator>>(istream &din, Matrix &mat);*/
private:
    size_t rows;
    size_t cols;
    double *matrix;

    // Random number engine
    static uint32_t generate_seed();
    static double get_random(double a,double b);
};
// Private static functions
uint32_t Matrix::generate_seed() {
    {
        std::random_device random;
        if (random.entropy() > 0.0) {
            return random();
        }
    }

    return std::chrono::high_resolution_clock::now().time_since_epoch().count();
}

//--------------------------------------------------------------------

double Matrix::get_random(double a, double b) {
    static std::mt19937 random(Matrix::generate_seed());
    std::uniform_real_distribution<double> double_dist{a, b};
    return double_dist(random);
}

//--------------------------------------------------------------------

Matrix::Matrix() {
    rows = 0;
    cols = 0;
    matrix = new double[rows * cols];
    for (size_t i = 0; i < rows; ++i) {
        for (size_t j = 0; j < cols; ++j) {
            *(matrix + i * cols + j) = 0;
        }
    }
}

//--------------------------------------------------------------------

Matrix::Matrix::Matrix(size_t r, size_t c, double v) {
    rows = r;
    cols = c;
    matrix = new double[rows * cols];
    for (size_t i = 0; i < rows; ++i) {
        for (size_t j = 0; j < cols; ++j) {
            *(matrix + i * cols + j) = v;
        }
    }
}

//--------------------------------------------------------------------

Matrix::Matrix(size_t r, size_t c, double *array) {
    rows = r;
    cols = c;
    matrix = new double[rows * cols];
    for (size_t i = 0; i < rows; ++i) {
        for (size_t j = 0; j < cols; ++j) {
            *(matrix + i * cols + j) = *(array + i * cols + j);
        }
    }
}

//----------------------------------------------------------------

Matrix::Matrix(const Matrix &mat) {
    rows = mat.rows;
    cols = mat.cols;
    matrix = new double[rows * cols];
    for (size_t i = 0; i < rows; ++i) {
        for (size_t j = 0; j < cols; ++j) {
            *(matrix + i * cols + j) = *(mat.matrix + i * rows + cols);
        }
    }
}

//------------------------------------------------------------------

void Matrix::randomize(double a, double b) {
    for (size_t i = 0; i < rows; i++) {
        for (size_t j = 0; j < cols; j++) {
            *(matrix + i * cols + j) = Matrix::get_random(a, b);
        }
    }
}

//-----------------------------------------------------------------------

Matrix Matrix::transpose() {
    Matrix result(cols, rows);
    for (size_t i = 0; i < result.rows; i++) {
        for (size_t j = 0; j < result.cols; j++) {
            *(result.matrix + i * result.cols + j) = *(matrix + j * cols + i);
        }
    }

    return result;
}

//-----------------------------------------------------------------------

void Matrix::display() {
    cout<<"[";
    for (size_t i = 0; i < rows; i++) {
        cout<<"[";
        for (size_t j = 0; j < cols; j++) {
            if (j != cols - 1) {
                cout<<pretty_print<<*(matrix + i * cols + j)<<", ";
            } else if (i != rows - 1 && j == cols - 1) {
                cout<<pretty_print<<*(matrix + i * cols + j)<<"],"<<endl<<" ";
            } else if (i == rows - 1 && j == cols - 1) {
                cout<<pretty_print<<*(matrix + i * cols + j)<<"]]"<<endl;
            }
        }
    }
    cout<<endl;
}

//-----------------------------------------------------------------------

Matrix Matrix::matMul(const Matrix& mat1, const Matrix& mat2) {
    if (mat1.cols == mat2.rows) {
        Matrix result(mat1.rows, mat2.cols);

        double sum; // mat1[i][k] * mat2[k][j];
        for (size_t i = 0; i < result.rows; i++) {
            for (size_t j = 0; j < result.cols; j++) {
                sum = 0;
                for (size_t k = 0; k < mat1.cols; k++) {
                    sum += *(mat1.matrix + i * mat1.cols + k) * *(mat2.matrix + k * mat2.cols + j);
                }
                *(result.matrix + i * result.cols + j) = sum;
            }
        }
        return result;
    } else {
        cout<<"Matrix multiplication is not possible!"<<endl;
        return Matrix();
    }
}
  • At very first: please have a look at rule of [three](https://stackoverflow.com/questions/4172722/what-is-the-rule-of-three)/[five](https://stackoverflow.com/questions/4782757/rule-of-three-becomes-rule-of-five-with-c11). I don't see a custom destructor and you'll suffer from a memory leak for not deleting the data on destruction. – Aconcagua May 23 '19 at 07:01
  • Hint: While having a pointer to empty array might be useful for not having to check for null pointers, you certainly don't need the loop to initialize... On the other hand, you don't need to duplicate the code if you use constructor deletation: `Matrix() : Matrix(0, 0) { }`... – Aconcagua May 23 '19 at 07:04
  • `mat.matrix[i * c + k]` would have been easier to read... – Aconcagua May 23 '19 at 07:08
  • 1
    Replace your `double*`, etc. by a `std::vector`, this will make your life much easier. – Holt May 23 '19 at 07:14
  • If you intended the raw arrays for accepting in the constructor an array that doesn't need to be *copied*, then with `std::vector` consider an r-value reference argument... – Aconcagua May 23 '19 at 07:24
  • At a first glance, the multiplication itself looks fine... Have you tested? Does it yield invalid results? I wouldn't cout in such basic functions as multiplication, though; additionally, an empty matrix is a valid one, so not best selection for sentinel value. Have you considered throwing an exception instead? – Aconcagua May 23 '19 at 07:36
  • You should learn to [debug your code](https://ericlippert.com/2014/03/05/how-to-debug-small-programs/). – L. F. May 23 '19 at 07:37
  • Sidenote: if you rename your function to `operator*`, users could have `Matrix m1, m2; Matrix m3 = m1 * m2;` – consider providing a move constructor, too, it will prevent the matrices from needlessly copy the arrays again and again; if you switch to std::vector, default move and copy constructors would be fine already. – Aconcagua May 23 '19 at 07:39
  • 1
    Typo? In `Matrix::Matrix(const Matrix &mat)`, the copy is not performed correctly : `*(matrix + i * cols + j) = *(mat.matrix + i * rows + cols);`. If I understand well, this copy is called at the end of the multiplication function. – Damien May 23 '19 at 07:58
  • @Aconcagua I have tested the code and it does throw invalid result which is not good. I do know the rule of three and was aware of the fact that I do not have a destructor in my code. I was about to incorporate it not before I got the problem and I hastily posted the question. Apologies for that. – Hardik Sharma May 23 '19 at 09:36
  • @Damien **Thanks** for pointing the mistake out. That was the actual problem causing for fussy results. – Hardik Sharma May 23 '19 at 09:41
  • @Aconcagua **Thanks** for giving me some advice that are indeed useful. – Hardik Sharma May 23 '19 at 09:42
  • @Damien As you pointed that at the return the copy constructor is called, Would it be better to return a reference to prevent that ? or should I leave that as it is ? – Hardik Sharma May 23 '19 at 09:53
  • The best solution is to use `std::vector` or `std::val_array` instead of a `double *`. Then you would not need to implement copy, move, destructor or operator=. They would be implicit and correct! If you need to stick on a `double *` (homework?), then you need to follow the rule-of-five. If you follow this rule, then returning a reference or not is a question of performance. Note that compiler optimization is more and more powerful and such by-hand optimisation is less and less useful. – Damien May 23 '19 at 10:02

0 Answers0