bkochuna / ners570f24-SpMV

3 stars 0 forks source link

Add ELL view tests #43

Open KyleVaughn opened 1 month ago

KyleVaughn commented 1 month ago

Description:

Added by @S-A-Almohri

This issue involves adding a suite of tests for the ELL view within the sparse matrix library to ensure its correctness and performance. The tests should verify that the ELL format handles various scenarios correctly, including edge cases, and performs as expected. An additional check with Python might be implemented using SciPy functions to validate results.

Tasks:

Added by @S-A-Almohri

Review Existing ELL Implementation:

Define Test Cases:

Basic functionality: Verify that simple, small matrices are correctly represented in ELL format. Edge cases: Empty matrices, matrices with a single element, symmetric matrices, and very sparse matrices. Performance: Measure the performance of SpMV using ELL format for larger matrices.

Implement Unit Tests:

Run and Validate Tests:

Definition of done:

Added by @S-A-Almohri

NiloyGupta-UM commented 1 month ago

I don't think performance tests fall under the scope of this issue; all these tests need to do is check that the implemented methods work and produce the expected behavior. Furthermore, assuming you are referring to sparse matrix-vector multiplication operations when writing "SpMV", that falls under another issue and should not be part of this one.

Also, specifically because ELL requires padding, making sure that the padded values are properly printed/written would be a good test. What "properly" means in this case might be defined in another issue concerning the ELL format or may need to be discussed by the group.

I think a good edge test case to add would be a matrix with a row of all zeros. Furthermore, a test should be added to check if matrix values accidentally get modified during the execution of the viewing methods. Otherwise, the cases you mentioned are a good set. It might also be beneficial to add a requirement that explicitly states that the library's testing framework should be utilized, but, depending on the group's opinion, this may already be an implicit one.

S-A-Almohri commented 1 month ago

Thanks for the thorough review. Sorry for not being clear, when I say SpMV I refer to the script, and by performance check against SciPy, I mean checking for time it takes to print and view each. The second bullet point just had that extra part, I changed it.

S-A-Almohri commented 1 month ago

@NiloyGupta-UM Ready for review

NiloyGupta-UM commented 1 month ago

@S-A-Almohri Can you create/link the appropriate pull request? If the planned merge will be part of a larger pull request, can you at least link me the relevant commit(s)?

S-A-Almohri commented 1 month ago

These are the commits: https://github.com/bkochuna/ners570f24-SpMV/commit/32411fbd42e439ac5edac846efd47316061dfa25 https://github.com/bkochuna/ners570f24-SpMV/commit/68d1b25f91e2bcade37dd239e40fe400dd33d2f3 The current ELL branch have my commits and they compile

NiloyGupta-UM commented 1 month ago

Since the current ELL branch does not have the assembleStorage method implemented yet, the tests do not fully run so at the moment I'm unable to verify that the tests work. However, from a review of the code, it looks good. Only thing I would suggest is to break up your big `ELLViewTest' test case into multiple smaller test cases for each of the tests you outlined in that function. It's not necessary, but makes the code easier to understand and more compartmentalized.

NiloyGupta-UM commented 1 month ago

I ran the tests. While they were able to run, at least test 1, 2, and 3 failed. 2 faield because the empty matrix was not declared as assembled. For the others, I am unsure if it because the output handling of the tests isn't working as intended or because the ELL view methods are not working correctly. This needs to be addressed before I can approve the pull request and close the issue.