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

Enter my Matrix

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

Problem

Here is my next Java assignment: create a Matrix class that can add and multiply two matrices together. I was given some code as a skeleton and then fleshed it out a bit.

Is there any way I could cut back on the amount of code present, or increase the performance of the multiplication method? And any other notes about my code would be greatly appreciated.

MatrixException.java:

package matrix;

/**
 * A MatrixException should be thrown when attempting to add or multiply 
 * matrices of incorrect dimensions.
 * @author syb0rg
 */
public class MatrixException extends RuntimeException {

    /**
     * Creates a MatrixException object with the indicated reason.
     * @param reason the reason why this MatrixException is being thrown.
     */
    public MatrixException(String reason) {
        super("Matrix error: " + reason);
    }

}


Matrix.java:

```
package matrix;

import java.io.BufferedReader;
import java.io.IOException;
import java.io.StringReader;
import java.util.Arrays;

/**
* An instance of this class represents a matrix of numbers
* as double values.
* @author syb0rg
*/
public class Matrix
{
private final double[][] matrix;
private final int rows;
private final int columns;
/**
* Creates a matrix with the indicated numbers of rows and columns.
* @param rows the number of rows in the matrix
* @param columns the number of columns in the matrix
*/
public Matrix(int rows, int columns)
{
matrix = new double[rows][columns];
this.rows = rows;
this.columns = columns;
}

/**
* Getter for the number of rows in this matrix.
* @return the number of rows in this matrix
*/
public int getRows()
{
return rows;
}

/**
* Getter for the number of columns in this matrix.
* @return the number of columns in this matrix
*/
public int getColumns()
{
return columns;
}

/**
* Gets the element at the ind

Solution

I will mostly focus on performance in this review because you have specifically asked for it and the other reviews did not cover it yet.

Performance

The first thing to do when optimizing for performance always is to measure. So I wrote a small benchmark for your matrix multiplication.

public static void main(final String[] args) {
    final int maxSize = 2048;
    final int datapoints = 50;
    final int warmups = 5;
    final Random rng = new Random();
    for (int i = 0; i = warmups) {
            System.err.printf("%8d %8d %8d %10f%n", n, m, l, seconds);
        }
        if (n > 0 && m > 0) {
            System.out.println(prod.get(rng.nextInt(n), rng.nextInt(m)));
        }
    }
}


It is generating a number of pairs of random matrices of compatible dimensions, computes their product and times the operation. After a warm-up phase for the JVM, the timing results are printed to standard error output. I'm also printing a random element of the result matrix to standard output to avoid the JIT compiler optimizing the entire product computation away.

You'll notice that I have added a static method randomMatrix to the Matrix class.

public static Matrix randomMatrix(final int rows, final int cols) {
    final Matrix matrix = new Matrix(rows, cols);
    final Random rng = new Random();
    for (int i = 0; i < rows; ++i) {
        for (int j = 0; j < cols; ++j) {
            matrix.set(i, j, rng.nextDouble());
        }
    }
    return matrix;
}


I have also made multiply a static method that takes two Matrixes because I find this more natural. If your assignment requires the non-static version, you can easily implement it in terms thereof.

public final Matrix multiply(final Matrix that) {
    return Matrix.multiply(this, that);
}


Now let's look at what can be improved.

Use a contiguous data layout (major)

You are currently storing the matrix data in a 2-dimensional array. This means that you're hopping through your address space as you traverse the matrix elements. It also means that you'll have a double indirect memory access for accessing a matrix element.

It would be better to store all the data in a 1-dimensional array and remap the 2-dimensional indices into that array. This way of storing a matrix is more natural than it might seem at first sight. Look at how you would write down a matrix on paper.

$$
\boldsymbol{A} = \left(
\begin{array}{cccc}
a_{11} & a_{12} & \cdots & a_{1m}\\
a_{21} & a_{22} & \cdots & a_{2m}\\
\vdots & \vdots & \ddots & \vdots\\
a_{n1} & a_{n2} & \cdots & a_{nm}\\
\end{array}
\right)
$$

If you were to fill in all the elements, you can either do it row-by-row or column-by-column. If you count how many elements you have already filled in, this will give you the index of the current element in the 1-dimensional array. Another way to think of this is to cut the paper into stripes such that you'll get either row or columns vectors, then glue all those stripes together to a long 1-dimensional stripe. Storing the matrix data as row vectors is known as row major order; storing it as column vectors as column major order. There is no real reason why one layout would be better than the other but once you've settled on one, it is important to write the matrix traversals with this layout in mind (see later). I will use row major order in the following because I find it more natural and it comes closer to how 2-dimensional arrays work in Java. That means that the matrix from above will have the following memory layout.

double A[] = {a11, a12, …, a1m, a21, a22, …, a2m, …, an1, an2, …, anm};


Add (syntactically insignificant) line breaks at the appropriate places…

double A[] = {
    a11, a12, …, a1m,
    a21, a22, …, a2m,
    …,
    an1, an2, …, anm,
};


…and you'll get back the familiar notation.

If we are given zero-based indices i and j for an n by m matrix, we can get the index of the matrix element in the array as i * m + j.

So you would replace your

final double[][] matrix;


attribute with a

final double[] data;


attribute and add a private helper function

private int getIndex(final int row, final int col) {
    return row * this.cols + col;
}


to map a 2-dimensional index into a 1-dimensional index in the data array.

(Forgive me for having renamed your columns attribute to cols. I couldn't get my fingers used to type the “umn” any more.)

Avoid unnecessary argument checking (minor)

Checking arguments in public functions is good but for your internal use, it adds mostly overhead. In Java, you'll get well-defined behavior for accessing an array out-of-bounds anyway so any additional check doesn't increase security but only produces more descriptive error messages. This is fine for client-facing methods but unneeded for internal use.

Therefore, implement private unchecked accessor methods

```
private double getUnchecked(final int row, final int col) {
return this.data

Code Snippets

public static void main(final String[] args) {
    final int maxSize = 2048;
    final int datapoints = 50;
    final int warmups = 5;
    final Random rng = new Random();
    for (int i = 0; i < datapoints + warmups; ++i) {
        final int n = rng.nextInt(maxSize);
        final int m = rng.nextInt(maxSize);
        final int l = rng.nextInt(maxSize);
        final Matrix lhs = Matrix.randomMatrix(n, l);
        final Matrix rhs = Matrix.randomMatrix(l, m);
        final long t0 = System.nanoTime();
        final Matrix prod = Matrix.multiply(lhs, rhs);
        final long t1 = System.nanoTime();
        final double seconds = 1.0e-9 * (t1 - t0);
        if (i >= warmups) {
            System.err.printf("%8d %8d %8d %10f%n", n, m, l, seconds);
        }
        if (n > 0 && m > 0) {
            System.out.println(prod.get(rng.nextInt(n), rng.nextInt(m)));
        }
    }
}
public static Matrix randomMatrix(final int rows, final int cols) {
    final Matrix matrix = new Matrix(rows, cols);
    final Random rng = new Random();
    for (int i = 0; i < rows; ++i) {
        for (int j = 0; j < cols; ++j) {
            matrix.set(i, j, rng.nextDouble());
        }
    }
    return matrix;
}
public final Matrix multiply(final Matrix that) {
    return Matrix.multiply(this, that);
}
double A[] = {a11, a12, …, a1m, a21, a22, …, a2m, …, an1, an2, …, anm};
double A[] = {
    a11, a12, …, a1m,
    a21, a22, …, a2m,
    …,
    an1, an2, …, anm,
};

Context

StackExchange Code Review Q#118923, answer score: 15

Revisions (0)

No revisions yet.