bkochuna / ners570f24-SpMV

3 stars 0 forks source link

Add CSR accessor methods #27

Open KyleVaughn opened 1 month ago

KyleVaughn commented 1 month ago

Description:

Accessor methods are used to retrieve the private attributes of a particular class and make them available for use elsewhere in the script. For the CSR Sparse Matrix format, these private attributes (based on Lec 12 Notes) include:

1. nrows - The total number of rows in the original matrix, size _sizet 2. ncols - The total number of columns in the original matrix, size _sizet 3. nnz - The number of non-zeroes in the original matrix, size _sizet 4. value - Array of values of the non-zero at original coordinate (I,J), size double or float 5. ColIdx - Array of column indices for each non-zero number, size _sizet 6. RowIdx - Array of row pointers indicating which aij value corresponds to the next row in the matrix, size _sizet 7. getCSRvalue - The value in a CSR matrix given by a user specified row and index value, size double or float

Public accessors ensure that the private attributes can be retrieved during execution of SpMV operations, while ensuring that the original attributes cannot be accidentally modified during script execution.

Tasks:

  1. Confirm naming scheme of private attributes with CSR class developer to ensure the same naming scheme is used for the private attributes

    • [x] Arranged meeting with CSR developer
    • [x] Confirmed attribute naming scheme
  2. Implement accessors in _SparseMatrixCSR.hpp for each of the private attributes. Each accessor should follow naming convention _get_attributename to allow for standardized naming. Comments should also be included to ensure that the implementations are properly explained and documented

    • [x] nrows accessor implemented
    • [x] ncols accessor implemented
    • [x] nnz accessor implemented
    • [x] values accessor implemented
    • [x] rowIdx accessor implemented
    • [x] colIdx accessor implemented
    • [x] getCSRvalue accessor implemented
  3. Verify performance of accessor with tester

    • [x] Informed tester that accessors are ready for testing and shared script
    • [x] Implemented changes according to feedback from testing
    • [x] Confirmed with tester that no additional changes are required
  4. Implement the finalized code

    • [ ] Push the script to the repository

Definition of done:

Each task has a corresponding checkbox. As tasks are completed, these checkboxes will be gradually checked off. Once each checkbox has been marked, then the task will be considered complete

MattGranados commented 1 month ago

@kaleisss Hello, I believe you are the reviewer for this issue. The issue has been updated and is now ready for review. Thanks in advance!

kaleisss commented 1 month ago

It looks pretty good! Your method is implemented with care and consistency, I don't have any complain.

AndrewPanter commented 1 month ago

Hi Matt, I am working on the unit tests for this implementation. Your description of the naming convention seems clear; let me know if you decide to change this from what is currently in the description. Please let me know when the implementation is ready for testing; I hope to have the tests ready by this evening.

MattGranados commented 1 month ago

Hi Andrew, I did actually change the naming convention based on what was already used in the class; I'll update the description shortly to reflect this, but in the meantime the current implementation CSR branch should have the accessors with the correct naming methods.

Likewise, there is another accessor that was not in the original description; its an accessor that takes in a row and col index and retrieves the respective value (tentatively called getCSRvalue()) I'll also update the description with this shortly.

I'll send another message once my overall implementation is ready, but I am tentatively planning to have it done something tonight!

AndrewPanter commented 1 month ago

Thanks Matt. I have updated the test I ran so far. I've implemented one unit test for ColIdx in the CSR_accessor_tests branch, in tests/CSR_accessor_unit_test.cpp. You should be able to merge the changes in; however, I might recommend doing this in a different branch than implementation_CSR, as I think there will be a merge conflict in the CMakeLists (I had to comment out another unit test which appears to be unfinished and was blocking compilation). I believe the test should compile, and I think it will have the behavior I intend once the accessor is implemented based on the header.

AndrewPanter commented 1 month ago

@MattGranados I have implemented the accessors for the attributes in the parent class; however, the tests to access rowIdx and colIdx produce a segmentation fault when they call the corresponding accessor. I think the issue may be in your implementation; I notice the first 3 accessors include this-> before the attribute on the right hand side to indicate that the assignment should be to an attribute of the current object, but the ones for the CSR specific vector attributes do not have this. I'd start looking here to resolve this. Let me know if you have questions.

MattGranados commented 1 month ago

@AndrewPanter I just looked into it a bit myself, and it looks like the reason behind the seg fault was that in the unit test, test_mat.assembleStorage(); needs to be called before attempting to use the accessor; when I tinkered with your unit test that seemed to remove the seg fault, but now it appears that the values just aren't equal as expected. Not quite sure if this is an issue with how the the values are assigned or if there's an issue in the unit test but I'll look into it and hopefully have an update during the stand up in an hour.

AndrewPanter commented 1 month ago

@MattGranados Thanks again for the help. Fixing the reference and assembling storage solved the faults, and all 5 tests I have now pass. I'll implement the values vector accessor test and then the values element accessor test next.

AndrewPanter commented 1 month ago

@MattGranados ValuesAccessor test passes. I have added two tests for the .getCSRvalue() method you mentioned; one to query a nonzero value, and one to query a zero value. All changes are pushed to the test branch. Ping me when that last method is implemented, please!

MattGranados commented 1 month ago

@AndrewPanter Sorry for being a tad late on it, but I made a new branch implementation-CSR_Accessors that contains the last getCSRvalue method; be sure to double check but your updated test script should already be in this branch. Let me know how the last tests go!

AndrewPanter commented 1 month ago

@MattGranados No worries. All tests pass; this should be ready to be pulled into the CSR branch.

kaleisss commented 1 month ago

It looks good to go.