HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Immutable Matrix

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
immutablematrixstackoverflow

Problem

I'm writing implementations of some numerical methods to solve linear equations systems, those implementations use the following Matrix class. I'm trying to get this class immutable, due to that the constructor and some methods create a copies of the backing two-dimesional array. The class have some basic functionality to operate NxM Matrix e.g. overloaded operators for sum, difference and product. The methods Equals(object) and HashCode() were stubbed by Resharper.
I'd would like to read any suggestions and improvements.

```
public class Matrix
{
public int M { get; private set; }
public int N { get; private set; }
private readonly double[,] _values;

public Matrix(double[,] values)
{
M = values.GetLength(0);
N = values.GetLength(1);
_values = (double[,])values.Clone();
}

public double this[int row, int column] {
get
{
ValidateRowIndex(row);
ValidateColumnIndex(column);
return _values[row, column];
}
}

public IEnumerable Column(int column)
{
ValidateColumnIndex(column);

for (int i = 0; i Row(int row)
{
ValidateRowIndex(row);

for (int j = 0; j
/// Method that will add a row times a factor to other row
/// in the system.
///
/// Row to be added
/// Row that will be added to
/// Times the row1 will be multiplied before be added
/// Result matrix
public Matrix AddRow(int row1, int row2, double factor)
{
ValidateRowIndex(row1);
ValidateRowIndex(row2);
if (factor == 0)
{
throw new ArgumentException("factor cannot be zero");
}

var result = new double[M, N];
var row = Row(row1).ToArray();

for (int i = 0; i n1 + n2);
}

public static Matrix operator -(Matrix a, Matrix b)
{
ValidateSameDimensions(a,

Solution

It would be better to do the faster checks first in this method:

protected bool Equals(Matrix other)
{
    return _values.Cast()
        .SequenceEqual(other._values.Cast())
        && M == other.M && N == other.N;
}


So we have this:

protected bool Equals(Matrix other)
{
    return M == other.M &&
           N == other.N &&
          _values.Cast().SequenceEqual(other._values.Cast());
}


Cast.SequenceEqual is nice and short, but there might be performance issues (you would need to test this). I think the more verbose option would be better in terms of clarity (and possibly performance):

if (M != other.M || N != other.N)
{
    return false;
}

for (int i = 0; i < M; i++)
{
    for (int j = 0; j < N; j++)
    {
        if (_values[i, j] != other._values[i, j])
        {
            return false;
        }
    }
}

return true;


There's no reason for Equals(Matrix) to be protected; in fact, if we make it public we can let Matrix implement IEquatable (not forgetting to add a null check).

On comparing doubles, I'll quote from MSDN:


The Equals method should be used with caution, because two apparently
equivalent values can be unequal due to the differing precision of the
two values. The following example reports that the Double value
.333333 and the Double value returned by dividing 1 by 3 are unequal.


...


Rather than comparing for equality, one technique involves defining an
acceptable relative margin of difference between two values (such as
.001% of one of the values). If the absolute value of the difference
between the two values is less than or equal to that margin, the
difference is likely to be due to differences in precision and,
therefore, the values are likely to be equal.

public static Matrix GetIdentityMatrix(int order)
{
    var identity = new double[order, order];
    for (int i = 0; i < order; i++)
    {
        for (int j = 0; j < order; j++)
        {
            if (i == j)
            {
                identity[i, j] = 1;
            }
        }
    }

    return identity;
}


We can write this in a more efficient way:

public static Matrix GetIdentityMatrix(int order)
{
    var identity = new double[order, order];
    for (int i = 0; i < order; i++)
    {
        identity[i, i] = 1;
    }

    return identity;
}


We should also be checking that order is positive.

Code Snippets

protected bool Equals(Matrix other)
{
    return _values.Cast<double>()
        .SequenceEqual(other._values.Cast<double>())
        && M == other.M && N == other.N;
}
protected bool Equals(Matrix other)
{
    return M == other.M &&
           N == other.N &&
          _values.Cast<double>().SequenceEqual(other._values.Cast<double>());
}
if (M != other.M || N != other.N)
{
    return false;
}

for (int i = 0; i < M; i++)
{
    for (int j = 0; j < N; j++)
    {
        if (_values[i, j] != other._values[i, j])
        {
            return false;
        }
    }
}

return true;
public static Matrix GetIdentityMatrix(int order)
{
    var identity = new double[order, order];
    for (int i = 0; i < order; i++)
    {
        for (int j = 0; j < order; j++)
        {
            if (i == j)
            {
                identity[i, j] = 1;
            }
        }
    }

    return identity;
}
public static Matrix GetIdentityMatrix(int order)
{
    var identity = new double[order, order];
    for (int i = 0; i < order; i++)
    {
        identity[i, i] = 1;
    }

    return identity;
}

Context

StackExchange Code Review Q#64474, answer score: 6

Revisions (0)

No revisions yet.