bkochuna / ners570f23-SpMV

1 stars 0 forks source link

Add assembleStorage and disassembleStorage methods for COO #36

Open KyleVaughn opened 11 months ago

KyleVaughn commented 11 months ago

Description:

Two methods must be created for the assembly and disassembly of a matrix in coordinate format. These methods will be included in the COO class and will allow for the conversion of data from a dense format to a sparse coordinate format.

Tasks:

Definition of done:

The two methods pass all unit tests to ensure the accurate conversion of matrix data and correct memory handling.

KyleVaughn commented 11 months ago

Review by @tjayasa

tjayasa commented 11 months ago

Hi @carltonjames, please fill out this issue sometime soon so that I can provide a review by the midnight deadline. Thanks!

carltonjames commented 11 months ago

@tjayasa Sorry about that. It is finished now.

tjayasa commented 11 months ago

Thanks for the quick response!

The description looks good, but you mention converting from a dense format to a sparse format, which I don't think is what the method is intended to do. The coefficients will be set and stored sparsely with the setCoefficients() method, so the assembleMatrix() will convert from this original sparse format to the COO format.

For the assembleMatrix tasks, I think the number of rows and columns will be stored as a class attribute, so you do not need to pass them in as input. Also, it may be good to ass in a task that mentions converting the matrix to the COO format as this is nontrivial.

For the disassembleMatrix tasks, the COO matrix will also be a class attribute, so you will not need to take that in as input.

I think the definition of done looks good.

Overall, this looks pretty good! Aside from all of the mentions of dense matrices, which I believe will not be used by these functions, this looks good!

tjayasa commented 10 months ago

@carltonjames Please add your implementations for the assembleStorage and disassembleStorage methods to the coo branch sometime soon so that I can review them and run my tests by tomorrow at 6pm.

carltonjames commented 10 months ago

@tjayasa I've added my implementations for those methods. You should be able to test them now.