Verbesserungsvorschläge für Matrix Klasse

[pospiech]

Ich habe eine Matrix Klasse geschrieben, um anderen an diesem Beispiel das Konzept von Operatorüberladungen zu erklären. Sie ist nicht auf Performance ausgelegt. Ich habe auch ein Beispiel mit der "Eigen" Biliothek die locker 1000 mal schneller ist. Trotzdem soll natürlich der Beispielcode correct und sinnvoll sein. Daher interessieren mich eure Anmerkungen.

matrix.h

#ifndef MATRIX_H_
#define MATRIX_H_

//#define NDEBUG // disables assert calls

#include <iostream> // ostream
#include <iomanip>
#include <stdlib.h> // rand, srand
#include <assert.h> // for assert
#include <cstddef>  // für size_t
#include <vector>   
using namespace std;

// 2D Array Functions, based on std::vector
const vector<vector<double> > make2DimVector(size_t FirstDim, size_t SecDim);
void resize2DimVector(vector<vector<double> > & vec, size_t FirstDim, size_t SecDim);

class matrix
{
public:
	matrix(int r, int c);
	matrix(const matrix &s);

	virtual ~matrix(void);

	matrix& operator =(const matrix &s);
	friend matrix operator*(const double & x, const matrix& s);
	friend matrix operator*(const matrix& s, const double & x);
	matrix operator*(const matrix & a);
	matrix operator+(const matrix & a);
	matrix operator-(const matrix & a);
	bool operator==(const matrix &other) const;
	bool operator!=(const matrix &other) const;

	vector<vector<double> > & data();
	vector<double>& operator[](const int i);

	void print(ostream &s) const;
	friend ostream & operator<<(ostream &s, const matrix &mat);

	matrix transpose();
	void clear();
	void fill();

private:
	vector<vector<double> > m;
	int size; // Zeilen und Spalten Anzahl (nur symmetrische Matrizen)
	int rows, cols;
	int elements; // Zeilen * Spalten

};
#endif

matrix.cpp

#include "matrix.h"

/* ---------------------------------------- *
 * --- 2D Vector Arrays ------------------- *
 * ---------------------------------------- */

 /*! Create 2D Vector Array */
const vector<vector<double> > make2DimVector(size_t FirstDim, size_t SecDim)
{
    vector< vector<double> > vec;
    resize2DimVector( vec, FirstDim, SecDim );
    return vec;
} 

/*! Resize 2D Vector Array */
void resize2DimVector(vector<vector<double> > & vec, size_t FirstDim, size_t SecDim)
{
    vec.resize( FirstDim );
    for(size_t i = 0; i < FirstDim; ++i)
        vec[i].resize( SecDim );
}

/* ---------------------------------------- *
 * --- Matrices of arbitrary sizes -------- *
 * ---------------------------------------- */

/*! Constructor */
matrix::matrix(int r, int c) 
	: rows(r), cols(c)
{
	elements = rows * cols;
	if (elements <= 0)
		throw "Matrix element count invalid";
	m = make2DimVector(rows, cols);
}

/*! Copy - Constructor */
matrix::matrix(const matrix &s)
{
	// create matrix of new sizes
	matrix(s.rows, s.cols);

	// copy data
	for(int i=0; i< rows; ++i)
	{
		for(int j=0; j< cols; ++j)
		{
			m[i][j] = s.m[i][j]; 
		}
	}
}

/*! Assignment Operator
 * Note: A reference is returned by the assignment operator to allow operator chaining,
 *       such as: a = b = c = d = e = 42;
 */
matrix& matrix::operator =(const matrix &s)
{
	// never assign to yourself
	if(this != &s)
	{
		// create matrix of new sizes
		matrix(s.rows, s.cols);

		// copy data
		for(int i=0; i< rows; ++i)
		{
			for(int j=0; j< cols; ++j)
			{
				m[i][j] = s.m[i][j]; 
			}
		}
	}
	// return reference
	return *this;
}

/*! Deconstructor */
matrix::~matrix(void)
{
	resize2DimVector(m, 0, 0);
}

/*! size of Matrix */
//int matrix::elements()
//{
//	return elements;
//}

/*! Access to internal data Array */
vector<vector<double> > & matrix::data()
{
	return m;
}

/*! Prints a matrix object (if it is small enough) */
void matrix::print(ostream &s) const
{
	if ((rows < 9) && (cols < 9))
	{
		for(int i=0; i<rows; ++i)
		{
			for(int j=0; j<cols; ++j)
			{
				s << setiosflags(ios::fixed) << setprecision(1) 
				  << setw(8) << m[i][j] << " ";
			}
			s << endl;
		}
		s << endl << endl;
	}
	else
		s << "matrix too large, " << elements << " Elements" << endl << endl;
}

/*! Overloaded << operator - for ease of printing */
ostream & operator<<(ostream &s, const matrix &mat)
{
   mat.print(s);
   return(s);
}

/*! Vector extraction */
vector<double>& matrix::operator[](const int i)
{
	assert(i>=0 && i < rows);
	return m[i];
}

///*! Multiplication with scalar*/
matrix operator*(const double & x, const matrix& s)
{
	matrix ans(s.rows, s.cols);
	for(int i=0; i<s.rows; ++i)
	{
		for(int j=0; j<s.cols; ++j)
		{
			ans.m[i][j] = x * s.m[i][j]; 
		}
	}
	return ans;
}

matrix operator*(const matrix& s, const double & x)
{
	return operator*(x, s);
}

/*! multiply internal matrix with external matrix */
matrix matrix::operator*(const matrix & a)
{
	// only if sizes are matched
	assert(cols == a.rows);

	matrix ans(a.rows, a.cols);
	// y-axis 
	for(int i=0;i<rows;++i)
	{
		// x-axis
		for(int j=0;j<a.cols;++j)
		{
			ans.m[i][j]=0.0;
			// k represents x and y position 
			// of scalar multiplication
			for(int k=0; k < cols; ++k)
			{
				ans.m[i][j] += m[i][k] * a.m[k][j];
			}
		}
	}
	return ans;
}

/*! Matrix Addition */
matrix matrix::operator+(const matrix & a)
{
	// only if sizes are equal
	assert(rows == a.rows);
	assert(cols == a.cols);

	matrix ans(a.rows, a.cols);
	for(int i=0; i<a.rows; ++i)
	{
		for(int j=0; j<a.cols; ++j)
		{
			ans.m[i][j] = m[i][j] + a.m[i][j]; 
		}
	}
	return ans;
}

/*! Matrix Subtraction */
matrix matrix::operator-(const matrix & a)
{
	// only if sizes are equal
	assert(rows == a.rows);
	assert(cols == a.cols);

	matrix ans(a.rows, a.cols);
	for(int i=0;i<rows;++i)
	{
		for(int j=0;j<cols;++j)
		{
			ans.m[i][j] = m[i][j] - a.m[i][j];
		}
	}
	return ans;
}

bool matrix::operator==(const matrix &other) const 
{
	// only if sizes are equal
	if ((rows != other.rows) || (cols != other.cols)) return false;	

	for(int i=0;i<rows; ++i)
	{
		for(int j=0;j<cols; ++j)
		{
			if (m[i][j]!=other.m[i][j])
			{
				return false;
			}
		}
	}
	return true;
}

bool matrix::operator!=(const matrix &other) const 
{
	return !(*this == other);
}

/*! Transpose */
matrix matrix::transpose()
{
	matrix ans(rows, cols);
	for(int i=0;i<rows;++i)
	{
		for(int j=0;j<cols;++j)
			ans[j][i]=m[i][j];
	}
	return ans;
}

/*! Set all matrix elements to zero */
void matrix::clear()
{
	for(int i=0;i<rows;++i)
	{
		for(int j=0;j<cols;++j)
			m[i][j] = 0.0;
	}
}

/*! Fills matrix elements with random data */
void matrix::fill()
{
	for(int i=0;i<rows;++i)
	{
		for(int j=0;j<cols;++j)
			m[i][j] = double(rand()/1000);
	}
	srand(rand());
}

[Mr Train]

Sorry, ich kann dir darauf keine Antwort geben, vielmehr hab' ich 'ne ähnliche Frage:
Ist es nicht generell schneller, für Vektoren/Matrizen keine Klassen, sondern Strukturen zu nehmen, weil sonst bei jeder Erzeugung (und das sind ja bei Vektoren locker mal einige 1000) ein Konstruktor aufgerufen wird?

[unskilled]

//#define NDEBUG // disables assert calls

wozu?

using namespace std;

niemals im header verwenden.
im Source kannst du es aber schreiben

im Header:

#include <iostream>
#include <cstddef> // für std::size_t !
#include <vector>

im Source:

#include <iomanip>
#include <cstdlib> // !
#include <cassert> // !

virtual ~matrix(void);

kann man zwar machen, aber mir würde gerade kein bsp einfallen, wo man eine matrix-klasse und polymorphie braucht - außerdem wird void in C++ nicht explizit in die Parameterliste geschrieben - falsch ist es zwar nicht, aber es ist nicht nötig und (deshalb) unüblich...
Außerdem brauchst du gar keinen Destruktor - std::vector macht das von allein

int elements; // Zeilen * Spalten

Prozessdaten würde ich niemals Speichern
Mach halt ne Funktion:

int GetElements() const {return rows * cols;}

Außerdem würde ich statt int eigtl immer std::size_t nehmen (wobei ich es noch immer doof finde, dass size_t im Namespace std liegt - und deshalb mein Standard-Header auch immer ein #include <cstdlib> und using std::size_t; enthalten ^^

int size; // Zeilen und Spalten Anzahl (nur symmetrische Matrizen)

Würde ich auch nicht machen - würde ich wieder über ne zusätzliche Funktion gehen...

/*! Matrix Addition */
matrix matrix::operator+(const matrix & a)
{
    // only if sizes are equal
    assert(rows == a.rows);
    assert(cols == a.cols);
/*...*/
}

Halte ich schlichtweg für falsch... ich würde ne exception werfen, falls die zeilen- und spaltenanzahl nicht gleich ist.
ich meine - man könnte es so machen, weil es in gewisser weise vll auch nen logikfehler ist - aber hätte dann das problem, dass man nie einfach mit der matrix rechnen kann sondern jedes mal selbst prüfen muss, ob die addition/subtraktion/... definiert ist...
Außerdem wird + eigtl immer als globaler operator definiert und über += (Member) definiert...

matrix(int r, int c);

besonders in headern sollte man dafür sorgen, dass der anwender IMMER genau sieht, wofür die Parameter sind - hier ist es vll noch grenzwertig aber ich würde trotzdem row_count und column_count oder so schreiben...

vector<double>& matrix::operator[](const int i)
{
    assert(i>=0 && i < rows);
    return m[i];
}

Und spätestens hier hätte man merken müssen, dass int doof ist und man doch nen unsigned-wert nehmen sollte (am praktischsten ist hier size_t - aber das hatten wir ja oben schon mal^^)
hier hingegen ist das assert richtig, weil es wirklich auf einen logik-fehler und keinen mathematischen fehler hinweist, dass man über array-grenzen hinweg auf daten zugreifen möchte...

friend matrix operator*(const double & x, const matrix& s);
    friend matrix operator*(const matrix& s, const double & x);
    matrix operator*(const matrix & a); //nicht *= ?

auch hier sei wieder der weg über globale operatoren genannt, also * über *=

ich hoffe, ich konnte dir etwas helfen - falls du bestimmte sachen nicht verstehst, frag halt noch mal nach ^^

@Mr Train
In C++ gibt es keinen Unterschied zwischen struct und class (außer den standard-access) {in C gibt es nur struct und kein class - aber das sei nur der vollständigkeit halber erwähnt}
Und zeig mir mal das Beispiel, wo die Performance zu großen Teilen in nem CTor verbraten wird...
Das es schneller (und effektiver) geht ist wohl klar, aber falls du dir wenigsten mal einen Satz durchgelesen hättest, wäre dir wohl auch aufgefallen, dass es egal ist ^^

Ich habe eine Matrix Klasse geschrieben, um anderen an diesem Beispiel das Konzept von Operatorüberladungen zu erklären. Sie ist nicht auf Performance ausgelegt.

bb

[pospiech]

Danke, werde ich in den nächsten Tagen einarbeiten. Zur Performance: Eigen ist bei 4x4 Matrizen bei dynamic size (wie in meiner Klasse) 10x schneller, bei fixed size 1000x schneller. Das dürfte jede Matrix Implementation in C (und solche wie diese in C++) hinter sich lassen.

[volkard]

unskilled schrieb:

assert(rows == a.rows);

Halte ich schlichtweg für falsch... ich würde ne exception werfen, falls die zeilen- und spaltenanzahl nicht gleich ist.

halte ich aber für grundlegend richtig. die exception wäre hier deplaziert.

[helferlein]

kommt darauf an, wenn der ctor nicht besonders groß ist (was bei vektoren ja eigentlich gegeben ist) wird der kompilier das eh inlinen. ein leerer ctor
würde ganz rausfallen.

class heißt nicht automatisch langam! der einzige unterschied zwischen klassen
und strukturen ist der zugriff. class ist standardmäßig private, struct ist
public. sonst ist alles möglich. (struct mit ctor, virtuellen funktonen etc.)

@topic

der destruktor kann ganz weg fallen, ein vector wird automatisch zerstört
wenn er den scope verlässt.

der konstruktor geht auch kürzer:

matirx::matrix(int r, int c) : m(vector<vector<double> >(r, vector<double>(c)))
{
    if (!r * c)
        // ...
}

die transpose funktion sollte eher auf dem eigenen objekt arbeiten, oder du
nennst die methode GetTranspose

[unskilled]

volkard schrieb:

unskilled schrieb:

assert(rows == a.rows);

Halte ich schlichtweg für falsch... ich würde ne exception werfen, falls die zeilen- und spaltenanzahl nicht gleich ist.

halte ich aber für grundlegend richtig. die exception wäre hier deplaziert.

int row_count, column_count;
std::cin >> row_count >> column_count;
matrix eins (row_count, column_count);
std::cin >> row_count >> column_count;
matrix zwei (row_count, column_count);

if(eins.rows == zwei.rows && eins.cols == zwei.cols)
{
  matrix drei = eins + zwei;
}

Finde ich extrem hässlich - hier würde das noch gehen - aber spätestens bei komplizierteren bzw. längeren Rechnungen sieht man dann echt nicht mehr durch...

Ob es nun ein Logik-Fehler oder doch eher ein mathematischer Fehler ist, kann ich auch nicht genau sagen... Aber ich würde sagen: Da diese beiden Vergleiche gegen x Additionen/Subtraktionen/... nicht ins Gewicht fallen, würde ich sie auch im release-Mode durchführen - und eine exception werfen, falls nötig.
Oder ist an der Überlegung irgendwo nen Fehler/Haken/...

bb

[volkard]

unskilled schrieb:

[...] Finde ich extrem hässlich - hier würde das noch gehen - aber spätestens bei komplizierteren bzw. längeren Rechnungen sieht man dann echt nicht mehr durch...

ich fürchte, die fälle, wo der benutzer die matrizen per hand eingibt, sind so unheimlich selten, daß es nicht ins gewicht fällt.
meistens werden die daten eingelesen (sofort beim oder nach dem einlesen auf kosistenz checken und nicht erstmal drauflosrechnen, besonders bei komplexeren berechnungen) oder durch berechnung erzeugt. und ich mag es, wenn programmierfehler möglichst früh und hart bestraft werden.

das allein sollte kein grund sein, in der matrix-klasse mit exceptions anzufangen. allerdings kommen beim invertieren eh exceptions ins spiel, weshalb es doch nicht tragisch ist, hier auch exceptions zu benutzen.

[unskilled]

volkard schrieb:

und nicht erstmal drauflosrechnen, besonders bei komplexeren berechnungen

Hmm... Und bei Templates (z.Bsp. beim Aufsummieren)? Willst du die alle extra für Matrizen spezialisieren? Ich würde das nicht wollen ^^
Die Überprüfungen vorm Rechnen sind mit Sicherheit nicht gerade die dümmste Idee - aber zwingen würde ich den User nicht dazu...

bb

[jep]

volkard schrieb:

unskilled schrieb:

assert(rows == a.rows);

Halte ich schlichtweg für falsch... ich würde ne exception werfen, falls die zeilen- und spaltenanzahl nicht gleich ist.

halte ich aber für grundlegend richtig. die exception wäre hier deplaziert.

volkard hat recht

[vlad_tepesch]

ich würde den speicher nicht mit vectoren von vectoren realisieren, sondern mit einem eindimensionalem raw-array.
Matrizen ändern ihre größe üblicherweise zur laufzeit meist sowiso nicht.
weiterhin würde ich die zugriffe nicht hart auf die indizes machen sondern mir noch 2 variabeln nehmen, die die x und y schrittweite kodieren:

T getElem(int x, int y
{

  return m_mem[x*m_xstep + y*m_ystep];  
}

bei initialisierung ist
xstep = 1 und
ystep = spaltenzahl.

das hat zwar den nachteil, dass die zugriffe ein wenig langsamer sind, hat aber den großen vorteil, dass du die matrix transponieren kannst, indem du nur die steps änderst.

xstep = spaltenzahl; und
ystep = 1;

ebendso känntest du eine zweite instanz erstellen, die auf den selben speicher zeigt, aber nur auf eine submatrix, ohne das speicher kopiert wird.

beispiel, würde auf diagonale zeigen:
xstep = 1;
ytep = spaltenzahl + 1

Ist natürlich immer die frage wofür man die matrixklasse braucht, und ob eine so komplexe Klasse sinn machr.

Edit:

zu der letzten diskussion würde ich auch zu exception tendieren. Warum?
ein assert ist nur eine debughilfe für den entwickler der klasse.
eine exception soll einen ungültigen laufzeitzustand signalisieren, der aber kein Fehler in der Klasse sondern in deren Benutzung darstellt.

[pospiech]

Zur Zeit kann ich gar nicht mehr daran Arbeiten das VS 2005 im Debug Modus zickt:
http://www.c-plusplus.net/forum/viewtopic-var-p-is-1640718.html