search

bkochuna / ners570f23-SpMV

1 stars 0 forks source link

CSR_view #67

Open zkjiaumich opened 10 months ago

zkjiaumich commented 10 months ago

The CSR_view method is ready for review.

lwh1106 commented 10 months ago

I would say it can work now with the mwe.cpp, accessor test, and view method test! However, I find the bugs in the assembly and disassembly functions but cannot fix them, so I just comment on these two functions. Sorry for the developer -- maybe you should try to figure this out before making a new push, otherwise other functions and tests cannot run...

jacob-umich commented 10 months ago

Reviewing the CSR storage methods Issue #52 This looks like a pretty good implementation. I would just ask you to consider 2 things

  1. check that the matrix is in the assembled state since your method relies on that/ it is part of the requirements.
  2. check with other view implementers to make sure your interfaces are consistent across the library
MeetMPatel commented 10 months ago

Reviewing the CSR accessor #44: The implementation is doing what it is supposed to do. looks great.

lwh1106 commented 10 months ago

I am reviewing the assembleStorage and disassembleStorage tests for CSR. #39 I think the tests look good, which try to test the float- and double-type CSR class. The test functions are easy to understand and also include the necessary terms associated with the assembleStorage and disassembleStorage functions. I think the student who works on this develops the test quite well, since there are some bugs in the function but the student still knows what he/she should do and uses the interface basically correctly.

zkjiaumich commented 10 months ago

Reviewing the CSR accessors test #48.

Looks good. You tested the getNumRows, getNumCols, getCoefficient, and getFormat, which match the first five items in your task. You also mentioned testing the number of nonzero entries. I do not think it is very necessary here, but it can be done to check the correctness more precisely.

Besides, you mentioned printing the error message for different tests. It would be better if you can add them.