program crashing because of loop?

[sexyzebra19]

My program keeps crashing and I can't see why, if I remove the line

myfile << data_[i+ndim_*j] << " ";

then it runs OK...but can't see anything wrong with this line?

void Matrix::WriteToFile(std::string fname) const
{
    ofstream myfile;
    myfile.open ( (fname+".txt").c_str() );
    for (int i = 0; i<mdim_; i++)
    {

    for (int j = 0; j<ndim_; j++)
    {
        
        myfile << data_[i+ndim_*j] << " ";
    }

    }
    myfile.close();
}
// Save matrix in file 'fname'

[jonsca]

myfile << data_[i+ndim_*j] << " "; should be myfile << data_[i+mdim_*j] << " "; Since you are jumping by a count of the number of rows each time you span a column. Do a quick "desk check" of it with pencil and paper to see.

[sexyzebra19]

myfile << data_[i+ndim_*j] << " "; should be myfile << data_[i+mdim_*j] << " "; Since you are jumping by a count of the number of rows each time you span a column. Do a quick "desk check" of it with pencil and paper to see.

Thanks Jonsca, but it is still crashing, still a problem with the WriteTo File function. Here is my full code:

#include<iostream>
#include<vector>
#include<cmath>
#include <fstream>
#include<string>
#include"matrixlib.h"

using namespace std;

int main()
{
    Matrix A(2);
    Matrix B(2);

    A.SetElement(1,1,1);
    A.SetElement(1,2,2);
    A.SetElement(2,1,3);
    A.SetElement(2,2,4);

    B.SetElement(1,1,5);
    B.SetElement(1,2,6);
    B.SetElement(2,1,7);
    B.SetElement(2,2,8);

    A.Add(B);
    A.WriteToFile("hello");

    return 0;
}

Matrix::Matrix(int rows, int cols)
{
    mdim_ = mdim_;
    ndim_ = ndim_;
    mdim_ = rows;
    ndim_ = cols;

    data_.resize (rows*cols);

    for (int i = 0; i < rows*cols; i++)
    {
		data_[i] = 0;
	}
}

Matrix::Matrix(int cols)
{
    ndim_ = ndim_;
    ndim_ = cols;

    data_.resize (cols*cols);

    for (int i = 0; i < cols*cols; i++)
    {
		data_[i] = 0;
	}
}

void Matrix::Add(Matrix b)
{
    for (int i = 0; i < data_.size(); i++)
    {
        data_[i] += b.data_[i];
        cout<<" "<<data_[i]<<" ";
    }

}
// Add Matrix b to this matrix


void Matrix::WriteToFile(std::string fname) const
{
    ofstream myfile;

    for (int i = 0; i<mdim_; i++)
    {

    for (int j = 0; j<ndim_; j++)
    {
        myfile.open ( (fname+".txt").c_str() );
        myfile << data_[i+mdim_*j] << " ";
    }

    }
    myfile.close();
}
// Save matrix in file 'fname'

void Matrix::SetElement(int m, int n, double d)
{
    data_[((n*ndim_)-1)-(ndim_-m)] = d;
}
// Set Element at position (m,n) to the value d

with header file

// matrixlib.h - A header file for a simple matrix library
#include<string>
#include<vector>

class Matrix {
 public:

  Matrix(int mdim, int ndim);
  // Create a zero matrix with mdim rows and ndim columns

  Matrix(int ndim);
  // Create an zero ndim x ndim matrix

  void Add(Matrix b);
  // Add Matrix b to this matrix

  void WriteToFile(std::string fname) const;
  // Save matrix in file 'fname'

  void SetElement(int m, int n, double d);
  // Set Element at position (m,n) to the value d

 private:
  int mdim_; // Number of rows of matrix
  int ndim_; // Number of columns of matrix
  std::vector<double> data_; // Vector storing the matrix data
};

[jonsca]

Scratch that I didn't uncomment the line. Hang on for a bit.

On line 63 change int to size_t so that the types match between the size() value and the index.

Also, definitely don't open your file each time through the loop move that open line up where the ostream is declared. That doesn't seem to be the issue however.

[jonsca]

Question:

mdim_ = mdim_;
ndim_ = ndim_;

what is the point of this? It doesn't affect your problem but it seems prone to error and completely unnecessary.

Which leads me to the next point of: look which constructor your are calling, the one taking columns. What is the ndim_ in that case? It's the same value as the non-initialzed ndim_ which is holding garbage.

[gashtio]

There are quite a few things wrong :).

- All the "mdim_ = mdim_;" stuff is useless.

- In the Matrix(int cols) constructor, mdim_ remains uninitialized and you're getting a crash - you should set it to cols.

- The SetElement() function is incorrect, it should read:

void Matrix::SetElement(int m, int n, double d)
{
	data_[(m - 1) * ndim_ + n - 1] = d;
}

- In WriteToFile(), the correct formula is:

myfile << data_[i * ndim_ + j] << " ";

The idea is that you've processed i rows, each having ndim_ columns, plus j more columns from the current row.

Also, you need to open the file just once, not in each innermost iteration.

And you should probably do some error checking when adding (are the matrices the same size), setting element (is it within bounds), etc.

[sexyzebra19]

Question:

mdim_ = mdim_;
ndim_ = ndim_;

what is the point of this? It doesn't affect your problem but it seems prone to error and completely unnecessary.

I had a similar problem with my program crashing earlier, see http://www.daniweb.com/forums/thread263455.html and i was advised to do this...is it not correct?

[jonsca]

It doesn't do anything the way you have it. That's not what AD meant.

Matrix::Matrix(int mdim_, int ndim_)
{
  this->mdim_ = mdim_;
  this->ndim_ = ndim_;

is very different than what you have there. This is saying take in two arguments mdim_ ndim_ via the constructor (which happened to have the same names as your internal variables but don't have to be) and set them to this->mdim_ and this->ndim_ (which are the internal variables). This way everything is initialized. As gashito reinforced there are variables left uninitialized the way you have it set up now.

[mitrmkar]

I had a similar problem with my program crashing earlier, see http://www.daniweb.com/forums/thread263455.html and i was advised to do this...is it not correct?

Jonsca already pretty much explained your current situation, but I still try to clearly point out what is wrong below ...

Matrix::Matrix(int rows, int cols)
{
// Here you are assigning the member variables mdim_ and ndim_
// to [I]themselves[/I]. It is of no practical use whatsoever.
mdim_ = mdim_;
ndim_ = ndim_;
// So simply delete the above two lines

// What is below is OK. Now you are initializing the Matrix object's
// dimensions in a proper way.
mdim_ = rows;
ndim_ = cols;

In the thread that you linked, the 'this' pointer was used in the constructor to make the code more readable (because the constructor's arguments where named identically to the actual member variables (mdim_ an ndim_), which is confusing).

[sexyzebra19]

Thank you both so much for your help, beginning to understand why programs crash in the first place now. All sorted now :)