Kritik an meiner Matrix-Klasse

Walli

Halo, ich habe heute morgen begonnen für ein Projekt eine Matrix-Klasse zu schreiben mit der man auch Rechenoperationen und (später) diverse Verfahren wie Gauss etc. anwenden kann. Bin noch nicht sonderlich weit aber ich würde gerne im Vorfeld schonmal Anregungen, Kritik etc. einholen, da diese Klasse echt stabil und schnell sein soll...

EDIT: Einige fixes
EDIT2: Der Code der Klasse steht nun korrigiert weiter unten auf dieser Seite!

mfg, MaSTaH

Jester

Irgendwie ist das immer das gleiche:

Bei der Multiplikation * mußt Du eine Kopie anlegen für den Rückgabewert, bei *= ist das hingegen nicht nötig (zumindest nicht bei der Multiplikation mit Skalaren), denn noch implementierst Du = mit * anstatt umgekehrt...
Den Rückgabe wert von operator würd ich const machen, dann kann man ihm nix zuweisen.

Für welche Datentypen ist die Matrix denn geplant?
Du übergibst das t ja als const T &, wenn ich da jetzt int einsetze (oder float oder double) ensteht das eher ungebrächliche const int &. Wenn Du planst eigene Zahlentypen zu verwenden ist das okay, wenn Du aber nur mit int float double &co. arbeiten willst wäre es eine Überlegung wert das const & wegzulassen.

Den operator* solltest Du außerdem nochmals anbieten mit linkem Argument T und rechtem Argument Matrix<T>, damit man auch von links mit Skalaren multiplizieren kann (einfach die andere Multiplikation aufrufen, nicht neu schreiben!). Aber die Klasse ist ja auch noch nicht fertig, deswegen ist es vielleicht ein bißchen blöd aufzufählen was noch fehlt.

MfG Jester

Christoph

Warum ist der Destruktor virtual?
Wenn er virtual ist, kann er nicht inline sein, da der call erst zur Laufzeit aufgelöst werden kann.

Walli

Wie gesagt, habe auf die schnelle heute morgen damit angefangen.
- *= habe ich blöd implementiert, stimmt.
- Das const T& habe ich aus eben diesem Grund gemacht weil man sie mit allem möglichen verwenden sollte. Ich denke, dass der Compiler das bei int auch wegoptimieren kann.

Walli

edit...

Walli

Sieht jemand den Fehler hier. Ich sehe ihn nicht... Muss irgendwas mit den Indizes zu tun haben...
m ist die Matrix mit der multipliziert werden soll...
Auszug aus operator*=:

Matrix rv(m_rows, m.m_cols);
for(unsigned i = 0; i < m_rows; ++i)
{
  for(unsigned j = 0; j < m.m_cols; ++j)
  {
    for(unsigned k = 0; k < m_cols; ++k)
      rv.m_rowsV[i][j] += m_rowsV[i][k] * m.m_rowsV[k][j];
  }
}

operator void

MaSTaH schrieb:

virtual ~Matrix(void)
	{
	}

Warum braucht ein konkreter Datentyp wie eine Matrix einen virtuellen Destruktor?
Und virtuelle Funktionen *können* inline sein. Wenn ich schreibe:

{
    Matrix m;
}

steht der Typ von m eindeutig fest und muss nicht aufgelöst werden. Aber was in einer Klasse deklariert wird ist implizit inline (also bei Multiply unnötig) und da Compiler sich meistens eh schon über die inline-Empfehlungen hinwegsetzen, ist das Keyword IMHO nur noch nützlich, damit man Funktionen in Headern definieren darf.

std::vector<Row> m_rows;

OK, normalerweise bin ich ein Gegner von übertriebener Optimierung, aber wenn die Klasse *wirklich* *sicher* schnell sein muss und die Klasse eh schon ein Template ist, kannst du die Anzahl der Spalten und Reihen eigentlich auch zu Templateparametern machen und ein statisches Array benutzen statt eines vectors von valarrays. Sollte mMn schneller sein, weil alles auf dem Stack allokiert werden kann und valarrays eh nicht so schnell sind, wie sie sein sollten.

Walli

valarrays sind in diesem Anwendungsfall aber ein wenig praktischer wenn man nachher den Gauss einbauen will. Ich glaube nicht, dass die viel in der Geschwindigkeit ausmachen. Wie sieht es denn mit dem Zugriff auf einzelne valarray-Elemente aus. Sind die schnell oder ist der Speicher nicht am Stück? Zu den Zeilen- und Spaltenanzahlen als Templateparameter: Das ganze sollte schon in gewissem Sinne dynaisch sein, sonst bekomme ich probleme mit der multiplikation. Ich kann die Templateparameter doch nicht zur Laufzeit angeben.

Ein Zwischenstand:

#pragma once
#include <vector>
#include <valarray>
#include <algorithm>

namespace cw
{
	enum MatrixExceptionCode
	{
		InvalidMultiplication,
	};

	class MatrixException
	{
		MatrixExceptionCode m_code;
	public:
		MatrixException(MatrixExceptionCode code)
			: m_code(code)
		{
		}
		MatrixExceptionCode Code()
		{
			return m_code;
		}
	};

	template<typename T>
		class Matrix
	{
	public:
		typedef std::valarray<T> Row;

		Matrix() : m_cols(0), m_rows(0)
		{
		}

		Matrix(unsigned rows, unsigned cols)
			: m_cols(cols), m_rows(rows), m_rowsV(rows)
		{
			ResizeRows();
		}

		Matrix(const Matrix& matrix)
			: m_cols(matrix.m_cols), m_rows(matrix.m_rows), m_rowsV(matrix.m_rows)
		{
		}

		virtual ~Matrix(void)
		{
		}

		T& operator()(unsigned row, unsigned col)
		{
			return m_rowsV[row][col];
		}

		const Row& operator[](unsigned row)
		{
			return m_rowsV[row];
		}

		const Matrix operator*(const T& t)
		{
			Matrix rv(*this);
			rv *= t;
			return rv;
		}

		Matrix& operator*=(const T& t)
		{
			for(std::vector<Row>::iterator it = m_rows.begin(); it != m_rows.end(); ++it)
				(*it) *= t;
			return *this;
		}

		const Matrix operator*(const Matrix& m)
		{
			Matrix rv(*this);
			rv *= m;
			return rv;
    }

		Matrix& operator*=(const Matrix& m)
		{
			if(m_cols != m.m_rows)
				throw MatrixException(InvalidMultiplication);

			Matrix rv(m_rows, m.m_cols);
			for(unsigned i = 0; i < m_rows; ++i)
			{
				for(unsigned j = 0; j < m.m_cols; ++j)
				{
					for(unsigned k = 0; k < m_cols; ++k)
						rv.m_rowsV[i][j] += m_rowsV[i][k] * m.m_rowsV[k][j];
				}
			}
			return (*this = rv);
		}

	private:
		unsigned m_cols, m_rows;
		std::vector<Row> m_rowsV;

		void ResizeRows()
		{
			for(std::vector<Row>::iterator it = m_rowsV.begin(); it != m_rowsV.end(); ++it)
				(*it).resize(m_cols, 0);
		}
	};

	template<typename T>
		const Matrix<T> operator*(const T& t, const Matrix<T>& m)
	{
		return (m * t);
	}
}
}

Es gibt noch ein Problem beim multiplizieren von 2 Matrizen. Ich komme aber einfach nicht drauf. Hat jemand eine Idee?

operator void

MaSTaH schrieb:

Ich glaube nicht, dass die viel in der Geschwindigkeit ausmachen. Wie sieht es denn mit dem Zugriff auf einzelne valarray-Elemente aus. [...] Zu den Zeilen- und Spaltenanzahlen als Templateparameter: Das ganze sollte schon in gewissem Sinne dynaisch sein, sonst bekomme ich probleme mit der multiplikation. Ich kann die Templateparameter doch nicht zur Laufzeit angeben.

Naja, std::vector (und wohl auch std::valarray) allokiert Speicher über new, was erstmal auf dem Heap eine freie Stelle sucht - ein statisches Array erscheint einfach auf dem Stack. Das macht die Erstellung von (temporären) Matrizen schon ein _gutes_ Stück schneller, würde ich mal raten.
Inwiefern muss bei der Multiplikation die Größe dynamisch sein? Ich bin ehrlich gesagt noch nicht bei Matrizen angekommen in der Schule, aber mit einer template-Member-Funktion sollte das gehen, denke ich...

Außerdem: Exceptions sollten imho immer von std::exception ableiten, damit man z.B. ganz außen um das Programm herum einmal std::exception& catchen und eine Meldung ausgeben kann.

Dann fehlt bei operator(unsigned, unsigned) noch eine const-Version, die eine konstante Referenz zurückgibt, damit man auch konstante Matrizen untersuchen kann.

Ich würde alle operator* als freistehende Funktionen deklarieren, damit es einheitlicher wird (und soweit ich weiß, hat das auch den Vorteil, dass man mit Typen arbeiten kann, die implizit in deine Matrix konvertierbar sind - aber das ist jetzt wohl eh seehr hypothetisch...). Ansonsten sollten die operator* innerhalb der Klasse const sein.

Walli

Nein, bei der Matrizenmultiplikation geht es nicht mit Zeilen- und Spaltenanzahl als Templateparameter. Die Zeilen- und Spaltenanzahl des Produktes von 2 beliebigen miteinander multiplizierbaren Matrizen lässt sich nicht zur Compilezeit vorhersagen.
Das Problem mit dem Array auf dem Stack ist die Stackgröße die ein großes Array sprengen könnte und (wie gesagt) die fehlende Dynamik, da man ROWS und COLS als Templateparameter mitgeben muss. An dynamischem Speicher kommt man nicht vorbei in diesem Fall, leider.