[
https://issues.apache.org/jira/browse/MATH-230?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12654003#action_12654003
]
Ismael Juma commented on MATH-230:
----------------------------------
I looked into this more carefully and I have a few comments (note that I don't
speak for commons-math, just an interested onlooker who might use it in the
near future):
- There have been a few commits after the patch was generated that make it not
apply anymore.
- I am not sure if extending RealMatrixImpl is the right approach. Stuff like
the following is a red flag.
public double[][] getData() {
throw new UnsupportedOperationException(
"Not supported because sparse matrices are usually chosen when " +
"the matrix to be modeled is too large to fit into memory, trying "
+
"to build the data array from this may result in an
OutOfMemoryException.");
}
Also, it seems to override most (if not all) of the methods. Is it perhaps
better to introduce an AbstractRealMatrix containing the shared methods and
have both RealMatrixImpl and SparseRealMatrix extend it? I notice that this
won't fix the getData() issue though, would it make sense to move that method
into the dense matrix? (only possible if 2.0 can break compatibility).
- The following code in SparseRealMatrixImpl.setEntry does not need the
containsKey call, remove can be called directly (avoiding an unnecessary
look-up):
// if an entry exists, remove it
if (entries.containsKey(point)) {
entries.remove(point);
}
There is another case of containsKey that is unnecessary because it's followed
by a get after it, but once the specialised map exists the code will be even
simpler so we can leave that for later.
> Implement Sparse Matrix Support
> -------------------------------
>
> Key: MATH-230
> URL: https://issues.apache.org/jira/browse/MATH-230
> Project: Commons Math
> Issue Type: Improvement
> Affects Versions: 2.0
> Environment: N/A
> Reporter: Sujit Pal
> Assignee: Luc Maisonobe
> Priority: Minor
> Fix For: 2.0
>
> Attachments: patch.txt, RealMatrixImplPerformanceTest.java,
> SparseRealMatrixImpl.java, SparseRealMatrixImplTest.java
>
>
> I needed a way to deal with large sparse matrices using commons-math
> RealMatrix, so I implemented it. The SparseRealMatrixImpl is a subclass of
> RealMatrixImpl, and the backing data structure is a Map<Point,Double>, where
> Point is a struct like inner-class which exposes two int parameters row and
> column. I had to make some changes to the existing components to keep the
> code for SparseRealMatrixImpl clean. Here are the details.
> 1) RealMatrix.java:
> - added a new method setEntry(int, int, double) to set data into a matrix
> 2) RealMatrixImpl.java:
> - changed all internal calls to data[i][j] to getEntry(i,j).
> - for some methods such as add(), subtract(), premultiply(), etc, there
> was code that checked for ClassCastException and had two versions,
> one for a generic RealMatrix and one for a RealMatrixImpl. This has
> been changed to have only one that operates on a RealMatrix. The
> result is something like auto-type casting. So if:
> RealMatrixImpl.add(RealMatrix) returns a RealMatrixImpl
> SparseRealMatrixImpl.add(RealMatrix) returns a SparseRealMatrixImpl
> 3) SparseRealMatrixImpl added as a subclass of RealMatrixImpl.
> 4) LUDecompositionImpl changed to use a clone of the passed in RealMatrix
> instead of its data[][] block, and now it uses clone.getEntry(row,col)
> calls instead of data[row][col] calls.
> 5) LUDecompositionImpl returned RealMatrixImpl for getL(), getU(), getP()
> and solve(). It now returns the same RealMatrix impl that is passed
> in through its constructor for these methods.
> 6) New test for SparseRealMatrixImpl, mimics the tests in RealMatrixImplTest,
> 7) New static method to create SparseRealMatrixImpl out of a double[][] in
> MatrixUtils.createSparseRealMatrix().
> but using SparseRealMatrixImpl.
> 8) Verified that all JUnit tests pass.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
