wo80 / CSparse.NET

A concise library for solving sparse linear systems with direct methods.
GNU Lesser General Public License v2.1
57 stars 25 forks source link

add a new constructor to CompressedColumnStorage and remove cloning from PermuteColumns #10

Closed bigworld12 closed 6 years ago

wo80 commented 6 years ago

In fact, cloning is needed:

int rows = 2;
int columns = 2;

var C = Converter.FromColumnMajorArray(new double[] { 1.0, 2.0, 3.0, 4.0 }, rows, columns);
var A = Converter.ToCompressedColumnStorage(C);
var Ap = A.Clone();

var actualColumn = new double[rows];
var expectedColumn = new double[rows];

var p = Permutation.Create(columns, -1);

Ap.PermuteColumns(p);

for (int i = 0; i < columns; i++)
{
    A.Column(p[i], expectedColumn);
    Ap.Column(i, actualColumn);

    // This will fail without cloning!
    CollectionAssert.AreEqual(expectedColumn, actualColumn);
}
bigworld12 commented 6 years ago

oh sorry i didn't notice the other 3 parameters were the outputs, cloning is still not needed, just 3 empty arrays would work

wo80 commented 6 years ago

Maybe the PermuteColumns method should return a new CompressedColumnStorage instance? This would make clear, that new memory will be allocated.

bigworld12 commented 6 years ago

then i suggest making the class CompressedColumnStorage non-abstract for it to have a constructor

wo80 commented 6 years ago

This would imply a lot of refactoring. You could use CompressedColumnStorage<T>.Create(...) for now. I'm not quite happy with this solution, but I also don't think, that the user should be able to create CompressedColumnStorage<T> objects.

bigworld12 commented 6 years ago

then i think this should be good