Operator*= from two instances (Matrix) gives bad results

Question

I have a class Matrix, and I'm trying to implement the method operator*=, which i use to make the product of two instances(matrix): m1*=m2.

I tried both method with friend and two parameter, and without friend and 1 parameter, but in both cases the results it's bad. Only with one parameter and the this use gives me results similar to the right (not always).

Tried with friend and 2 parameter, and without friend and 1 parameter. Tried returning directly the first matrix, m1, also creating a temporary matrix m3

My private members:

int N, M;      
T** data;
bool Init(const int N_, const int M_);
void Clear();

(i'm using init to initialize the matrix/bidimensional array):

bool Matrix::Init(const int N_, const int M_) {
  this->Clear(); //dealloco comunque prima di inizializzarla
  N = N_;
  M = M_;
  if (N <= 0 || M <= 0) {
      cerr << "Non posso generare una matrixe: " << N <<"x"<< M << endl;
      return false;
  }
  else if (N > 0 && M > 0) {
      data = new T*[N];
      for (int i = 0; i < N; i++) {
        data[i] = new T[M];
      }
  }
return true;

}

My operator *= mathod (no friend, 1 parameter):

Matrix& Matrix::operator*=(const Matrix& m2) {

    float operation = 0;
    int N_ = (this->N < m2.N) ? this->N : m2.N;
    int M_ = (this->M < m2.M) ? this->M : m2.M;
    Matrix m3(N_, M_);

    if (this->N != m2.M || this->M != m2.N) {
        this->Set(0, 0, flag_stop);
    }
    else {

        for (int i = 0; i < this->N; ++i) {
            for (int j = 0; j < m2.M; ++j) {
                for (int k = 0; k < this->M; ++k) {
                    operation = operation + (this->Get(i,k) * m2.Get(k,j)) ;
                    //cout << " Cout m1 su "<< i<< ","<<k<<":"<< this->Get(i,k) << "\t " << " Cout m2: "<< m2.Get(k,j) << endl;
                    this->Set(i, j, operation);
                }

                //cout << operation << "\t";
                operation = 0;
            }
            operation = 0;
        }
    }
    return *this;
}

In the main, when i try to use the operator*=:

Matrix m1(i1,j1);
Matrix m2(i2,j2);

//operator*=
    cout << endl;
    m1*=m2;
    int N_ = (m1.GetN() < m2.GetN()) ? m1.GetN() : m2.GetN();
    int M_ = (m1.GetM() < m2.GetM()) ? m1.GetM() : m2.GetM();

    for (int i = 0; i < N_; ++i) {
        for (int j = 0; j < M_; ++j) {
            cout << m1.Get(i,j) << "\t";
        }
        cout << endl;
    }

It's all the day that i try, but the results are not rights i also tried with m1[2][2] and m2[2][2], with m1[3][2] and m2[2][3], etc... but nothing. Someone have had a similar problem?

Hoping to have right product of two matrixes, but i have, at the major times, big numbers (expected 5, obtained 30), or the first column right numbers, and the second not

Please post a [mcve] reproducing your problem as required here. — πάντα ῥεῖ, Jan 12 '19 at 20:49
Because `*=` modifies the left hand side, it should be a member function. Don't waste your time fighting with a free-or `friend` function over this one. [Good reading on the topic found here](https://stackoverflow.com/questions/4421706/what-are-the-basic-rules-and-idioms-for-operator-overloading). — user4581301, Jan 12 '19 at 20:52
By the way, allocating the rows separately is an inefficient way to represent a dense matrix. It's better to allocate all of the rows in a flat array once. — eerorika, Jan 12 '19 at 20:56
Recommendation: Step through the function with your development environment's debugger (get a new development environment if yours doesn't have a debugger) and watch what's happening. Follow along by doing the math yourself on a pen and paper. When you catch the program doing something different from what you do, odds are good you just found the bug. — user4581301, Jan 12 '19 at 20:57
@Yksisarvinen Because that's what matrix multiplication is? https://en.wikipedia.org/wiki/Matrix_multiplication — Lightness Races in Orbit, Jan 12 '19 at 21:02

score 2 · Answer 1 · answered Jan 13 '19 at 16:35

The reason for mistakes you report seems to be the multiplication algorithm itself. Basically, your multiplication code is as follows:

for (int i = 0; i < this->N; ++i) {
    for (int j = 0; j < m2.M; ++j) {
        for (int k = 0; k < this->M; ++k) {
            operation = operation + (this->Get(i,k) * m2.Get(k,j)) ;
            this->Set(i, j, operation);
        }
        operation = 0;
    }
}

Your algorithm modifies the original matrix just in the process of the calculation (this->Set() call) so when you call this->Get(i,k) for any i < j you obtain not an original value of the first matrix at ith row and kth column but a value that was already modified by this->Set() call. This apparently leads to wrong results.

In order to solve this you must ensure that you use original matrix values for your calculations, for example, by making a copy of the original matrix or (more optimal) of the currently modified row of the original matrix.

Operator*= from two instances (Matrix) gives bad results

1 Answers1