Open KyleVaughn opened 3 weeks ago
@KyleVaughn this is one that one of us will need to pick up. I'm not sure who has more time... let's briefly discuss today and try to get some movement.
@swame2000 this description is ready for review.
I'll begin prototyping code while I wait for feedback. When you are satisfied with the issue description, please mark the issue as accepted.
@bkochuna I am not clear on what's the expansion for DBC. Otherwise, the description looks solid.
Kindy note that ColIdx should be type int since padding requires -1 value for colIdx. Please implement accordingly/clarify what colIdx for padded elements is in case size_t type for colIdx is retained.
@bkochuna I am not clear on what's the expansion for DBC. Otherwise, the description looks solid.
DBC = Design by Contract (description updated for clarity).
Kindy note that ColIdx should be type int since padding requires -1 value for colIdx. Please implement accordingly/clarify what colIdx for padded elements is in case size_t type for colIdx is retained.
This is a good observation (I'd forgotten about this detail). However, I do not think you have to use -1 as the padded index in the colIdx
variable, and thus require an int data type here. I think the padded locations can simply use the largest column index in the row +1, and the val array has a zero. This is still a sane representation of the matrix. The value at this location is in fact 0. We choose this +1 because I think it should facilitate better cache access.
Please check me on this, but I think this would be a better solution because it avoids mixing data types.
Edit: this won't work if there is a nonzero in the last column. Another special case is needed for this...
I have taken colIdx for padded element as (size of matrix)+1. So size_t tye for colIdx can be retained.
Created the branch ELL-assemblage
for this issue
@swame2000 I think this is ready for review.
Some important notes on the implementations. The unit test written by @MoonBH-UoM for #53 reveals a subtle question which necessitates clarification of some behavior/function requirment that impacts a LLD detail of the base class.
The question at hand revealed by thest is: how do we want to handle calls to setCoefficient
where the value passed is 0?
I think the answer should be that we want to only store non-zeros and not store values where the client has explicitly added a zero. Hence 2 should be the requirment/expected behavior.
This allows the all0_but1
test case to pass.
The other implementation detail here is I had originally included a DBC assert that assembleStorage
should only be called when _nnz > 0
. However, the all0
expects the behavior of assembleStorage
in the case of _nnz == 0
to be that the return returns with the matrix in an assembled state but with no allocated data. So, the assert statement was changed to an if/else.
Edit: oops. code is not merged to main yet, but it is in the PR with a passing test. Asside from that detail, I think this is done.
@bkochuna I have reviewed the methods. No obvious errors were identified. The code is very readable. While it was easy to follow the logic of the assembling and disassembling, might be beneficial if comments were added that give intuition behind the logic of the indexing for quick understanding. No further comments at this time. Thanks.
Description:
The
SparseMatrix
base class defines an abstract method, forassembleStorage
. To have a functioning concrete derived class for the ELLPACK matrix storage format, this routine must be defined. The purpose of this routine is convert the representation of the matrix data from thestd::map
container defined on the base class (specifically the_buildCoeff
attribute) into the ELLPACK sparse matrix storage format.Because we are to support conversion of the matrix from the "assembled" state back to a "building state" we also require a
dissassemble
procedure that converts the ELLPACK storage format back into thestd::map
container that is_buildCoeff
.The ELLPACK matrix storage format finds the row with the largest number of nonzeros and allocates a 2D array with the number of rows in the array equal to the number of rows in the matrix (if there is a row with all zeros we in the matrix we store all the zeros), and the number of columns in the array is equal to the maximum number of zeros in a row for all rows in the matrix.
There is also a column index array that maps the column of the ELLPACK array to the column of the matrix.
Tasks:
assembleStorage
method on theSparseMatrix_ELL
class that converts the representation of data in_buildCoeff
into the ELLPACK sparse matrix storage format.dissassembleStorage
method on theSparseMatrix_ELL
class that performs the inverse operation ofassembleStorage
Definition of done:
The aforementioned tasks are completed. Any comments during the code review are satisfactorily addressed, the code is merged to main. Lastly, the unit test written in #53 passes.