Details

Type: Improvement

Status: Closed

Priority: Major

Resolution: Fixed

Affects Version/s: 0.5

Fix Version/s: 0.6

Component/s: None

Labels:None
Description
We have a bunch of redundant methods in our matrix interface. These include things that return views of parts of the matrix:
Matrix viewPart(int[] offset, int[] size); Matrix viewPart(int rowOffset, int rowsRequested, int columnOffset, int columnsRequested); Vector viewRow(int row); Vector viewColumn(int column);
and things that do the same but call refer to getting stuff
Vector getColumn(int column); Vector getRow(int row); double getQuick(int row, int column); int[] getNumNondefaultElements(); Map<String, Integer> getColumnLabelBindings(); Map<String, Integer> getRowLabelBindings(); double get(String rowLabel, String columnLabel);
To my mind, get implies a getbyvalue whereas view implies getbyreference. As such, I would suggest that getColumn and getRow should disappear. On the other hand, getQuick and get are both correctly named.
This raises the question of what getNumNondefaultElements really does. I certainly can't tell just from the signature. Is it too confusing to keep?
Additionally, what do people think that getColumnLabelBindings and getRowLabelBindings return? A mutable map? Or an immutable one?
Under the covers, viewRow and viewColumn (and the upcoming viewDiagonal) have default implementations that use MatrixVectorView, but AbstractMatrix doesn't have an implementation for getRow and getColumn.
In sum, I suggest that:
 getRow and getColumn go away
 the fancy fast implementations fo getRow and getColumn that exist be migrated to be overrides of viewRow and viewColumn
 there be a constructor for AbstractMatrix that sets the internal size things correctly.
 that the internal cardinality array in AbstractMatrix goes away to be replaced by two integers.
 viewDiagonal() and viewDiagonal(length) and viewDiagonal(row, column) and viewDiagonal(int row, column, length) be added.
I will produce a patch shortly.

 MAHOUT790.patch
 197 kB
 Ted Dunning
Issue Links
 blocks

MAHOUT792 Add new stochastic decomposition code
 Closed
 is blocked by

MAHOUT793 Move MurmurHash to math
 Closed
 is depended upon by

MAHOUT796 Modified power iterations in existing SSVD code
 Closed

MAHOUT797 MapReduce SSVD: provide alternative Bpipeline per B=R' ^{1} Y'A
 Closed
Activity
 All
 Comments
 Work Log
 History
 Activity
 Transitions
Ideally labels should follow the rows and/or columns as the matrix pivots. Since the pivoted matrix is just a view, this should be easy to make happen. It probably doesn't happen correctly now.
Matrix row&column labels are string>index maps. If you write code that knows it has a permuted matrix, it can pull the labels and the permutation lists and do the indirection. If it does not know it has a permuted matrix, it will get nontracking outputs.
Lance,
Ideally labels should follow the rows and/or columns as the matrix pivots. Since the pivoted matrix is just a view, this should be easy to make happen. It probably doesn't happen correctly now.
Yes, the inverse of a nonzero diagonal is easy. Commonly a diagonal matrix with zeros is truncated before inverting. The definition of zero is an application specific thing and thus should not be included in the DM itself.
Inverting a diagonal matrix is 1/the values in the diagonal. This is trivial if all the diagonals are nonzero, but impossible if any are 0. Should DiagonalMatrix track whether any values are 0?
What should happen to labels in PivotedMatrix.java? Should they point to the row number as they do now? Should they track the movements of rows & columns?
Found the missing commit. It had other changes as well.
Applied them. Should be fixed (finally)
Really fixed now.
Lost a change. Reopening to commit the fix.
Definitely due to last minute regression. My local history shows that this
changed right as I checked stuff in.
Hmph... that test should have been commented out.
Let me take a look. My final merges may not have been quite right.
Hm. i can't seem to make trunk to build.
building math produces
Results :
Failed tests:
testIterate(org.apache.mahout.math.TestSparseColumnMatrix): iterator:
, randomAccess:
{2:5.5,1:3.3,0:1.1}expected:<
{0:1.1,1:2.2}> but was:<
{2:5.5,1:3.3,0:1.1}>
Integrated in MahoutQuality #1012 (See https://builds.apache.org/job/MahoutQuality/1012/)
MAHOUT790  kill the cardinality array and size() for matrices. Use rowSize() and columnSize() instead.
MAHOUT792  Fix RTM to avoid size() and cardinality array.
MAHOUT790  More get/view changes
MAHOUT790  collapse get
, kill addTo
MAHOUT790  Fixed copyright and license on QRDecomposition and SVDDecomposition
MAHOUT790  Copyright format cleanup
MAHOUT790  Matrix cleanups.
MAHOUT790  Add some vector and matrix types to simplify certain manipulations.
MAHOUT790  Add view for diagonal of a matrix.
tdunning : http://svn.apache.org/viewcvs.cgi/?root=ApacheSVN&view=rev&rev=1164016
Files :
 /mahout/trunk/core/src/main/java/org/apache/mahout/cf/taste/hadoop/als/eval/InMemoryFactorizationEvaluator.java
 /mahout/trunk/core/src/main/java/org/apache/mahout/classifier/bayes/InMemoryBayesDatastore.java
 /mahout/trunk/core/src/main/java/org/apache/mahout/classifier/discriminative/LinearTrainer.java
 /mahout/trunk/core/src/main/java/org/apache/mahout/classifier/naivebayes/NaiveBayesModel.java
 /mahout/trunk/core/src/main/java/org/apache/mahout/classifier/naivebayes/training/TrainUtils.java
 /mahout/trunk/core/src/main/java/org/apache/mahout/classifier/sequencelearning/hmm/HmmUtils.java
 /mahout/trunk/core/src/main/java/org/apache/mahout/math/MatrixWritable.java
 /mahout/trunk/core/src/main/java/org/apache/mahout/math/hadoop/decomposer/EigenVerificationJob.java
 /mahout/trunk/core/src/main/java/org/apache/mahout/math/hadoop/stochasticsvd/UpperTriangular.java
 /mahout/trunk/core/src/test/java/org/apache/mahout/cf/taste/hadoop/als/ParallelALSFactorizationJobTest.java
 /mahout/trunk/math/src/main/java/org/apache/mahout/math/AbstractMatrix.java
 /mahout/trunk/math/src/main/java/org/apache/mahout/math/DenseMatrix.java
 /mahout/trunk/math/src/main/java/org/apache/mahout/math/DiagonalMatrix.java
 /mahout/trunk/math/src/main/java/org/apache/mahout/math/Matrix.java
 /mahout/trunk/math/src/main/java/org/apache/mahout/math/MatrixView.java
 /mahout/trunk/math/src/main/java/org/apache/mahout/math/PermutedVectorView.java
 /mahout/trunk/math/src/main/java/org/apache/mahout/math/PivotedMatrix.java
 /mahout/trunk/math/src/main/java/org/apache/mahout/math/RandomAccessSparseVector.java
 /mahout/trunk/math/src/main/java/org/apache/mahout/math/RandomTrinaryMatrix.java
 /mahout/trunk/math/src/main/java/org/apache/mahout/math/SequentialAccessSparseVector.java
 /mahout/trunk/math/src/main/java/org/apache/mahout/math/SparseColumnMatrix.java
 /mahout/trunk/math/src/main/java/org/apache/mahout/math/SparseMatrix.java
 /mahout/trunk/math/src/main/java/org/apache/mahout/math/SparseRowMatrix.java
 /mahout/trunk/math/src/main/java/org/apache/mahout/math/decomposer/hebbian/HebbianSolver.java
 /mahout/trunk/math/src/test/java/org/apache/mahout/math/AbstractTestVector.java
 /mahout/trunk/math/src/test/java/org/apache/mahout/math/MatrixTest.java
 /mahout/trunk/math/src/test/java/org/apache/mahout/math/TestMatrixView.java
 /mahout/trunk/math/src/test/java/org/apache/mahout/math/TestSparseColumnMatrix.java
 /mahout/trunk/math/src/test/java/org/apache/mahout/math/TestSparseMatrix.java
 /mahout/trunk/math/src/test/java/org/apache/mahout/math/TestSparseRowMatrix.java
 /mahout/trunk/math/src/test/java/org/apache/mahout/math/TestVectorView.java
 /mahout/trunk/math/src/test/java/org/apache/mahout/math/als/AlternateLeastSquaresSolverTest.java
 /mahout/trunk/math/src/test/java/org/apache/mahout/math/decomposer/SolverTest.java
tdunning : http://svn.apache.org/viewcvs.cgi/?root=ApacheSVN&view=rev&rev=1164015
Files :
 /mahout/trunk/core/src/main/java/org/apache/mahout/clustering/lda/LDAInference.java
 /mahout/trunk/math/src/main/java/org/apache/mahout/math/AbstractMatrix.java
 /mahout/trunk/math/src/main/java/org/apache/mahout/math/DiagonalMatrix.java
 /mahout/trunk/math/src/main/java/org/apache/mahout/math/PivotedMatrix.java
tdunning : http://svn.apache.org/viewcvs.cgi/?root=ApacheSVN&view=rev&rev=1164014
Files :
 /mahout/trunk/core/src/main/java/org/apache/mahout/cf/taste/hadoop/als/eval/InMemoryFactorizationEvaluator.java
 /mahout/trunk/core/src/main/java/org/apache/mahout/classifier/AbstractVectorClassifier.java
 /mahout/trunk/core/src/main/java/org/apache/mahout/classifier/discriminative/LinearTrainer.java
 /mahout/trunk/core/src/main/java/org/apache/mahout/classifier/naivebayes/NaiveBayesModel.java
 /mahout/trunk/core/src/main/java/org/apache/mahout/classifier/naivebayes/training/WeightsMapper.java
 /mahout/trunk/core/src/main/java/org/apache/mahout/classifier/sgd/AbstractOnlineLogisticRegression.java
 /mahout/trunk/core/src/main/java/org/apache/mahout/classifier/sgd/GradientMachine.java
 /mahout/trunk/core/src/main/java/org/apache/mahout/classifier/sgd/PassiveAggressive.java
 /mahout/trunk/core/src/main/java/org/apache/mahout/clustering/AbstractCluster.java
 /mahout/trunk/core/src/main/java/org/apache/mahout/clustering/RunningSumsGaussianAccumulator.java
 /mahout/trunk/core/src/main/java/org/apache/mahout/common/mapreduce/VectorSumReducer.java
 /mahout/trunk/core/src/main/java/org/apache/mahout/math/hadoop/MatrixMultiplicationJob.java
 /mahout/trunk/core/src/main/java/org/apache/mahout/math/hadoop/TimesSquaredJob.java
 /mahout/trunk/core/src/main/java/org/apache/mahout/math/hadoop/stochasticsvd/GivensThinSolver.java
 /mahout/trunk/core/src/main/java/org/apache/mahout/math/hadoop/stochasticsvd/SSVDPrototype.java
 /mahout/trunk/core/src/main/java/org/apache/mahout/math/hadoop/stochasticsvd/UJob.java
 /mahout/trunk/core/src/main/java/org/apache/mahout/math/hadoop/stochasticsvd/UpperTriangular.java
 /mahout/trunk/core/src/main/java/org/apache/mahout/math/hadoop/stochasticsvd/VJob.java
 /mahout/trunk/core/src/main/java/org/apache/mahout/vectorizer/common/PartialVectorMergeReducer.java
 /mahout/trunk/core/src/test/java/org/apache/mahout/cf/taste/hadoop/als/ParallelALSFactorizationJobTest.java
 /mahout/trunk/core/src/test/java/org/apache/mahout/classifier/discriminative/PerceptronTrainerTest.java
 /mahout/trunk/core/src/test/java/org/apache/mahout/classifier/discriminative/WinnowTrainerTest.java
 /mahout/trunk/core/src/test/java/org/apache/mahout/classifier/sgd/OnlineBaseTest.java
 /mahout/trunk/core/src/test/java/org/apache/mahout/clustering/TestGaussianAccumulators.java
 /mahout/trunk/core/src/test/java/org/apache/mahout/math/hadoop/decomposer/TestDistributedLanczosSolverCLI.java
 /mahout/trunk/core/src/test/java/org/apache/mahout/math/hadoop/stochasticsvd/SSVDPrototypeTest.java
 /mahout/trunk/examples/src/main/java/org/apache/mahout/clustering/display/DisplayMeanShift.java
 /mahout/trunk/integration/src/test/java/org/apache/mahout/clustering/TestClusterDumper.java
 /mahout/trunk/math/src/main/java/org/apache/mahout/math/AbstractMatrix.java
 /mahout/trunk/math/src/main/java/org/apache/mahout/math/AbstractVector.java
 /mahout/trunk/math/src/main/java/org/apache/mahout/math/Algebra.java
 /mahout/trunk/math/src/main/java/org/apache/mahout/math/DenseMatrix.java
 /mahout/trunk/math/src/main/java/org/apache/mahout/math/DenseVector.java
 /mahout/trunk/math/src/main/java/org/apache/mahout/math/Matrix.java
 /mahout/trunk/math/src/main/java/org/apache/mahout/math/MatrixVectorView.java
 /mahout/trunk/math/src/main/java/org/apache/mahout/math/MatrixView.java
 /mahout/trunk/math/src/main/java/org/apache/mahout/math/NamedVector.java
 /mahout/trunk/math/src/main/java/org/apache/mahout/math/RandomAccessSparseVector.java
 /mahout/trunk/math/src/main/java/org/apache/mahout/math/SparseColumnMatrix.java
 /mahout/trunk/math/src/main/java/org/apache/mahout/math/SparseMatrix.java
 /mahout/trunk/math/src/main/java/org/apache/mahout/math/SparseRowMatrix.java
 /mahout/trunk/math/src/main/java/org/apache/mahout/math/Vector.java
 /mahout/trunk/math/src/main/java/org/apache/mahout/math/VectorView.java
 /mahout/trunk/math/src/main/java/org/apache/mahout/math/als/AlternateLeastSquaresSolver.java
 /mahout/trunk/math/src/main/java/org/apache/mahout/math/decomposer/hebbian/HebbianSolver.java
 /mahout/trunk/math/src/main/java/org/apache/mahout/math/decomposer/hebbian/TrainingState.java
 /mahout/trunk/math/src/test/java/org/apache/mahout/math/MatrixTest.java
 /mahout/trunk/math/src/test/java/org/apache/mahout/math/TestMatrixView.java
 /mahout/trunk/math/src/test/java/org/apache/mahout/math/VectorTest.java
 /mahout/trunk/math/src/test/java/org/apache/mahout/math/decomposer/SolverTest.java
tdunning : http://svn.apache.org/viewcvs.cgi/?root=ApacheSVN&view=rev&rev=1164013
Files :
 /mahout/trunk/math/src/main/java/org/apache/mahout/math/QRDecomposition.java
 /mahout/trunk/math/src/main/java/org/apache/mahout/math/SingularValueDecomposition.java
tdunning : http://svn.apache.org/viewcvs.cgi/?root=ApacheSVN&view=rev&rev=1164012
Files :
 /mahout/trunk/math/src/test/java/org/apache/mahout/math/TestSingularValueDecomposition.java
tdunning : http://svn.apache.org/viewcvs.cgi/?root=ApacheSVN&view=rev&rev=1164011
Files :
 /mahout/trunk/math/src/test/java/org/apache/mahout/math/MatrixTest.java
tdunning : http://svn.apache.org/viewcvs.cgi/?root=ApacheSVN&view=rev&rev=1164010
Files :
 /mahout/trunk/math/src/main/java/org/apache/mahout/math/ConstantVector.java
 /mahout/trunk/math/src/main/java/org/apache/mahout/math/DiagonalMatrix.java
 /mahout/trunk/math/src/main/java/org/apache/mahout/math/PermutedVectorView.java
 /mahout/trunk/math/src/main/java/org/apache/mahout/math/PivotedMatrix.java
 /mahout/trunk/math/src/test/java/org/apache/mahout/math/PermutedVectorViewTest.java
 /mahout/trunk/math/src/test/java/org/apache/mahout/math/PivotedMatrixTest.java
tdunning : http://svn.apache.org/viewcvs.cgi/?root=ApacheSVN&view=rev&rev=1164009
Files :
 /mahout/trunk/math/src/main/java/org/apache/mahout/math/AbstractMatrix.java
 /mahout/trunk/math/src/main/java/org/apache/mahout/math/Matrix.java
Checked in. We will want to make sure that Jenkins concurs that it works.
OK. Just committed this.
When you update, make sure to do [mvn install] in the math module to make sure that you get the update for the rest of Mahout (that threw me for a loop for a while).
But it does show that perhaps viewPart should be done outside of Matrix.
I think not. Getting a view of a submatrix or row or column or diagonal is a fundamental operation in linear algebra. The method may delegate to a view class, but to the user, it should appear as a matrix operation.
Besides, there are are kinds of matrices and vectors where the view is the same type as the matrix. For instance, for dense matrices this is often true because the dense matrix is a onedimensional storage array combined with an offset plus row and column strides. Any blockwise view of this keeps the same storage and has different offset and strides.
On the other hand, sparse matrices do better with a view structure.
In any caes, viewPart should be a method on the matrix. It should return a matrix and preserve sparsity and maybe a few related properties, but not precise type.
I don't necessarily think m.like() and m.viewPart().like() return the same class. I might well expect that m and m.viewPart() are of the same class! which would make this true.
Ouch that twisted my head. But it does show that perhaps viewPart should be done outside of Matrix. You pick a viewer class and give it the delegate Matrix in the constructor. If it's not linear algebra, should it be in Matrix?
@Dmitriy,
I completely agree. We need to get the basic API's rock solid as soon as possible.
I like this idea of annotating stuff very much. Maybe it would also make sense to apply that in a broader way to highlight which implementations are mature and productionready (like most of our recommender code) and which are rather new and experimental (like the graph mining module).
the thing is, i like Mahout's DRM and Matrix apis very much and use them pervasively. I think they are cornerstone for everything else and for custom pipelining. It would be great if we could make them stable rather sooner than later
Thanks, Ted.
I am working off a frozen snapshot in production (just built my own private release of a suitable functionality snapshot i use), so it's no immediate problem. At some point we will refactor. No problem. But it might help in other places where i have less of idea how in flux the api is.
So Dmitriy, how badly are the changes I am pushing going to hit you?
I am about to add a merge of numCols and columnSize as well (same for numRows).
Great idea. I doubt it would have helped here since I thought this interface was pretty stable.
Maybe it would focus our minds as we add the @stable annotation.
It might help to introduce interface maturity annotations (similar to what they do in Hadoop) to indicate our opinion of stillevolving apis to the user.
I have tons of outside code locked to the Matrix api already. I probably would've used it anyway even if it were marked as evolving. but we definitely have various levels of api maturity. So it might help to indicate it.
I may misunderstand: Do you expect m and m.viewPart() to be the same class in an ideal world or not? I don't expect that, myself, and indeed that's not the case. So all the less would I expect that m.like() and m.viewPart().like() ought to be the same class. I thought you were suggesting they ought to be.
I agree with your last point here so I think we must be agreeing.
I don't necessarily think m.like() and m.viewPart().like() return the same class. I might well expect that m and m.viewPart() are of the same class! which would make this true.
You might expect that, but you would be wrong.
Seriously, I would expect that mostly like() should preserve isSparse, but not necessarily much else. Much better is to have like() encode the judgement of the implementor and look below any facades to the truth of the matter.
I am not touching clone for now.
I know the clone() contract well – why wouldn't, as you say, like() + assign() satisfy the contract? That's why I questioned the objection that, well, every class has to implement it. I don't think so. If like() does the hard part of figuring out what class to return, this is a breeze. It would be nice to have clone() even if it can be accomplished with like() and assign() as a convenience method, to match developer expectations.
I don't necessarily think m.like() and m.viewPart().like() return the same class. I might well expect that m and m.viewPart() are of the same class! which would make this true.
erm, in terms of actionable changes, I think I was arguing against more change rather than for more, so proceed and we can sort it later. Don't remove clone() unless it's really painful given the road you've gone down with this patch.
Github branch MAHOUT790 available at git://github.com/tdunning/mahout.git
That provides more details on successive changes.
Sean,
We definitely need both operations. The first can be expressed, however, as a.like().assign(a) so it isn't quite as necessary as it might seem.
The problem with clone itself is that there are serious restrictions on how you have to do it based on Java requirements. That makes it a royal pain some days of the week. This may be easier after this JIRA gets resolved since the only information at AbstractMatrix level is the number of rows and columns and they are trivial to deal with.
We definitely also have some bugs in our test suite in that it is assumed that like() has to return the same type of object. That isn't really true. For instance, m.viewPart(0,3,2,5).like() should return the same thing that m.like() returns. But viewPart probably returns some kind of view object so these aren't the same.
I can deal with those issues another day.
If we can get eyes on this monster patch, I would like to commit it shortly. The only big issues I know about are:
a) getRow and getColumn is a more common name than viewRow and viewColumn. Does everybody promise not to be confused by the loss of getRow?
b) what should the iterator of a matrix do? Right now SparseColumnMatrix iterates over columns and everything else by rows. Unless there is some very clear way to tell what the iteration is doing, I would like to go on record as grumpy about that.
On clone() vs like(): there are two logical operations we might want to support here. There's an operation that gives a separate, identical copy with all the same values. There's also an operation that gives a separate, identical copy with no data or values.
The first should certainly be called clone(), since that's what it is, in Java.
Lance, is your objection that we simply should not have the first operation? or that you don't want to use clone() per se for some reason?
I don't see that either of these two has an easier time deciding what class to return, or that one or the other must or must not be implemented in subclasses. These are like any other OO method.
I can imagine both are useful, and so would support keeping both. If someone has a good argument that one or the other really isn't used, that's good too. And certainly if we're finding they're implemented incorrectly, and I did find several instances of that in the past, we should fix it.
yeah, I hate our use of clone as well. But I am not going to change it on this pass. I have already touched 80 files with > 200 changes. That will be enough to commit cleanly.
The diagonal support consists of a viewDiagonal method on Matrix on the one hand and a DiagonalMatrix implementation on the other. As the name suggests viewDiagonal is a view method so it would be bad to make it readonly. It does make certain operations like computing the determinant very simple:
determinant = new CholeskyDecomposition(matrix).viewDiagonal().aggregate(Functions.TIMES)
or setting the diagonal of a matrix to all zeros:
m.viewDiagonal().assign(0)
The diagonal matrix is handy when reconstructing SVD's. You get this:
u.times(new DiagonalMatrix(singularValues)).times(v.transpose())
Since the DiagonalMatrix is marked as sparse, efficiency is good.
addTo v.s. apply(Functions.PLUS)
If performance is really a problem, the apply implementation can do instanceof, but keep the interface clean.
While you've got the scissors out, I would reconsider clone().
 clone() requires every subclass to implement its own version
 The "which class do I use for the clone()" problem is handled better by like().
It really helps some algorithms to be able to pull the primary diagonal of a matrix out as a vector. So yes, that is needed.
This sounds like a utility method? The different Matrix data structures may want to have different implementations of viewing it; I can see a disastrous clash between a 'sequential' Matrix and pulling diagonals in one go. It may be one of those cases where each use of this is somewhat customized, and the surrounding code knows the matrix implementation. That is, an algorithm for sequential matrices is carefully coded around this fact, and so how it uses a diagonal will also have this profile.
So, static utility method and "you know the problem space" are the two uses for this?
I go on about this because I tried to make a generic readonly Matrix and Vector, and then random subclasses of those. This exercise showed the design tensions so I'm now wary of adding more features which subclasses must consider.
This build should fail for now. The fix will be forth coming.
Here is a monster patch that cleans up the matrix classes as suggested. The remaining nit is the iterator in SparseColumnMatrix.
I have also found an odd function called slice(int). It seems to be used variously to describe a column or row view. This nonspecificity seems disastrous to me so I am deleting that function and replacing it with viewRow.
Can somebody say what this is for?
Jake? Were you involved with this? It seems to appear in the distributed row matrix stuff a fair bit.
I have also found a number of instances where from.addTo(to) is used instead of to.assign(from, Functions.PLUS). I am changing these usages to the latter form and removing addTo as redundant?
Any comment on that change?
So the getRow/getColumn vs viewRow/viewColumn merge exercise is turning out good. I have found a number of bugs that relate to the confusion between whether getRow returned a copy or not.
But I am also finding that getRow is much more popular than viewRow. My tendency is to still change the name to make clear that the result is a view.
Any thoughts?
Diagonals: are they really needed now?
It really helps some algorithms to be able to pull the primary diagonal of a matrix out as a vector. So yes, that is needed.
Regarding a DiagonalMatrix, I do have a need for that as well and will include that in the SSVD patch. Since that is a pure addition, I don't think it needs the same level of discussion.
Should there be triangular or symmetric? They have enough of their own behavior to be a separate subclass rather than some magic thing held by the main class.
Possibly. So far, this isn't a big deal although I do have a triangular solver in my CholeskyDecomposition. If we see some more uses, it might be worth pulling out as a separate class.
I don't see that a SymmetricMatrix is an important thing yet. Yes, there are important mathematical properties there, but these aren't necessarily something worth reflecting in the type structure. Good use cases would change my mind.
Example: DiagonalMatrix.invert() is a valid method, because it either blows up if there is a 0 value, or returns 1/values.
I prefer solve methods rather than invert methods. It is already much too hard to cure people of trying to invert matrices. Introducing an invert method anywhere would just make that harder.
viewColumn v.s. getColumn: the question here is deep v.s. shallow copy?
I would go with
getColumn(int column) and getColumn(int column, Vector v)
where getColumn(int column) is assumed to give a shallow copy.
Neither of these produce a copy. Both return references. Your reaction is similar to mine that get implies a copy. I don't plan to add code to create a copy so I plan to just reduce current function to a single entry point.
Cardinality array: definitely it is mutable from outside.
final int row;
final int column;
viewColumn v.s. getColumn: the question here is deep v.s. shallow copy?
I would go with
getColumn(int column) and getColumn(int column, Vector v)
where getColumn(int column) is assumed to give a shallow copy.
Diagonals: are they really needed now? Should there be triangular or symmetric? They have enough of their own behavior to be a separate subclass rather than some magic thing held by the main class. Example: DiagonalMatrix.invert() is a valid method, because it either blows up if there is a 0 value, or returns 1/values.
getNumNondefault: this requires being able to produce the number, which is a "design load". It is not used much in "real code". I suspect most of those users could track/deduce the number in some other way, rather than expect the object to remember it.
Ted looks like you are done with this one?