Den Branch

[marchier]

The pull request adds the DEN Methods (ex. accessor, matvec, assemble/disassemble storage, etc.) and DEN Tests to the DEN Branch

[Lazy-Beee]

Review of DEN matrix class https://github.com/bkochuna/ners570f23-SpMV/issues/16 (@nzamb222):

The DEN matrix class is created but the constructor did not initialize _nrows, _ncols, and other necessary variables.

[sthvkbht]

Review of Add assembleStorage and disassembleStorage methods for DEN #34

The methods need to update the MatrixState as per the defined enum. Some bugs need fixing (like mistyped variables names when accessing).

[nzamb222]

Review of DEN matrix class #16 (@nzamb222):

The DEN matrix class is created but the constructor did not initialize _nrows, _ncols, and other necessary variables.

_nrows and _ncols are initialized in the SparseMatrix.hpp code? Fixed and initialized other required variables such as A dense matrix

[nzamb222]

It appears all DEN pull requests are combined into this pull request? I do not see any other DEN branches. If you are a contributor, comment a link to your PR or create a new branch and PR. Also remember to add your respective reviewers so it is easy for them to find. I have included a list of issues I believe this PR is for (correct me if I'm wrong).

• Add a DEN matrix class #16
• Add DEN accessor methods #42
• Add DEN accessor tests #46

[Lazy-Beee]

Review of DEN matrix class #16 (@nzamb222): The DEN matrix class is created but the constructor did not initialize _nrows, _ncols, and other necessary variables.

_nrows and _ncols are initialized in the SparseMatrix.hpp code? Fixed and initialized other required variables such as A dense matrix

My bad, I didn't notice that these two are initialized in the parent class. The constructor does not need to do anything since we can choose to initialize A in assembleStorage. Your original implementation looks good XD

The destructor and other parts of the issue also look good. By the way, did you add the #pragma once in the SparseMatrix_DEN.hpp? It's kind of duplicate with #ifndef __SPMV570_DEN__? It's also okay if you keep both though.

[shujhan]

Review of the view method #50 Method looks good, but I think when you want to get an element, it should use this->_buildCoeff(make_pair(row, column))

[webberqu417]

Review for DEN view tests #54 @MeetMPatel :

The provided code is well-structured and makes use of templated test cases. However, a notable concern is memory management, where memory allocated with new is not freed, leading to potential memory leaks. Additionally, while the code includes file handling for verifying test outputs, the process feels redundant, especially for small matrices—direct in-memory comparisons might be more efficient. Error handling can also be improved; rather than abruptly exiting, leveraging the testing framework's reporting mechanisms would be more suitable.

[nzamb222]

review of issue #27 @webberqu417 Implementation looks good, however you need to add it to the Cmake list in the spmv/tests directory so that it will compile and run.

[preetb1199]

Review on matvec implementation:

The implementation looks correct and concise.