bkochuna / ners570f24-SpMV

3 stars 0 forks source link

Add a JDS martrix class #6

Open KyleVaughn opened 3 weeks ago

KyleVaughn commented 3 weeks ago

Description:

Add a JDS matrix class, which is a child class of the SparseMatrix class.

Amendment by @S-A-Almohri The primary goal of this issue is to create a new class JDSMatrix as a child class of SparseMatrix to support efficient sparse matrix-vector multiplication (SpMV). This class will provide constructors for initializing the matrix and methods necessary for performing efficient SpMV. Integration with existing matrix operations in the SparseMatrix superclass is essential. Detailed documentation and usage examples for the two main methods (viewing and multiplication) will be provided to assist the tester. The class will assume that the input matrix is read in Coordinate (COO) format.

Tasks:

Added by @S-A-Almohri Design the JDSMatrix Class:

Implement Constructors:

Implement Core Methods:

Integrate with SparseMatrix:

Definition of done:

Must include a default constructor and a constructor which takes one argument per member variable and directly assembles the class.

Amendment by @S-A-Almohri

bjorngkierulf commented 1 week ago

I'm reviewing this. The issue looks good, though you could clarify that for this issue, you don't need full implementations of all the methods you listed like view and matrix vector multiply.

S-A-Almohri commented 5 days ago

@bjorngkierulf Ready for review

bjorngkierulf commented 2 days ago

@S-A-Almohri Sorry for the delay, overall, this looks good. Here are my thoughts:   For the variable names - personally, I would try to find more intuitive variable names, mostly for perm and jd_ptr. That said, those are the names in the Lec13 UML, so I'll leave it up to you if you change these.   For the parametrized constructor - I guess this is fine, but I don't see the use case where a user would want to use this. You could probably keep this as is, if you can also add a parametrized constructor that only takes nrows and ncols, because I think that's the more common use case.   For the matvec: The interface you have now should work, but we should try to standardize the interface across child classes. Eg. for COO, matvec takes a std::vector and returns a std::vector. Either way is fine, but we should discuss and standardize, so that the (hypothetical) user doesn't need to use a different interface based on the matrix's storage format.   For the destructor - I think the std::vector's should destroy themselves automatically, but I don't think max_nz_row will.   But overall I think this is good. I haven't tried compiling this so I don't know if there are any errors there.

S-A-Almohri commented 9 hours ago

Good points but since dependent codes are compiling I think we shouldn't change it; I left that for the matvec implementer, the code is not compiling because of an error in matvec for now after that got implemented by the implementor (I think his loop ordering could be wrong but that's a different unrelated issue)

Update:

Now the code compiles with no errors/warnings