bkochuna / ners570f23-SpMV

1 stars 0 forks source link

coo branch #59

Open Lazy-Beee opened 10 months ago

Lazy-Beee commented 10 months ago

This PR includes the following issues

Feel free to add your issue to the list. You can also link your issue to this PR in the Development section.

zkjiaumich commented 10 months ago

Review COO constructor implementation #17

Looks good. The code compiles to initiate the class object. It takes variables in nrows, ncols, nnz. The assembleStorage, dissembleStorage and other methods are created.

lwh1106 commented 10 months ago

I am reviewing the test for the COO view method. #51 The COO view method looks great! It prints out the non-zero row and column indices, and the values to the given path. I just suggest that the format can be mentioned first, such as writing some comments in the code. Otherwise, someone else may argue to print out the entire matrix. As far as I am concerned, either way works, and it is up to the standard. Overall, it is great, and the test for it is also good!

MeetMPatel commented 10 months ago

I am reviewing the test for the COO view method. #51 The COO view method looks great! It prints out the non-zero row and column indices, and the values to the given path. I just suggest that the format can be mentioned first, such as writing some comments in the code. Otherwise, someone else may argue to print out the entire matrix. As far as I am concerned, either way works, and it is up to the standard. Overall, it is great, and the test for it is also good!

Thanks, I have added comments describing how the .out file would look.

Lazy-Beee commented 10 months ago

Review of COO matvec implementation:

The function does what it is supposed to. However, there are no checks within the function to assert that the size of the vectors being input are correct given the dimensions of the sparse matrix. Perhaps a couple more comments would be nice too.

Thanks for your suggestion. I also wanted to check the size of the arrays, but I couldn't find a good way to find the length of the array from the pointer. It would be much easier if I used std::vector, but I wanted to match the programming style and eventually chose the c-style array. If you have any idea about this, please let me know XD.

Lazy-Beee commented 10 months ago

@carltonjames Seems something went wrong in the assembling process (or between construction and assembleStorage), failing all the tests containing the assemble method. -> Fixed in https://github.com/bkochuna/ners570f23-SpMV/pull/59/commits/35b1753b5f4752ca94ea3936f9c8647014c71478

Lazy-Beee commented 10 months ago

@sthvkbht Should the matvec result in your test be [7,6,19] instead of [7,6,23]? I have made this small fix https://github.com/bkochuna/ners570f23-SpMV/pull/59/commits/1c447e3244b8118c93c9f75a2c3634cef34288ba and the matvec can pass the test now

[1 0 2
0 3 0 * [1 2 3]^T = [7 6 19]^T 4 0 5]

Aravind-Kulanthaivelu commented 10 months ago

I am reviewing the COO accessor method: The implementation looks good. All the necessary requirements have been covered.