Assignment0

[ickbumk]

Pull request for code review

[zacharyknowlan]

Reviewer: Zach Knowlan
Developer: Ickbum Kim
Date: 01/22/2025

General Code Review for Assignment 0

Does the style match?

• Yes, the style of the code is consistent throughout.

Is there documentation of the new code? Does it address what it does and how to use it?

• Documentation is existent in the README.md file and provides instructions to compile each file individually.

Are there any obvious bugs, or performance issues?

• All functions return the vector or vector of vectors which in C++ makes an unnecessary copy and slows each function significantly. I would highly recommend returning either a pointer (if instance is allocated dynamically to heap using new) or modifying the functions to take a reference to a previously initialized vector or vector of vectors which is modified within the function and return nothing (void).

Is the code easy to read and understand? Is it well organized? Are function names clear?

• Function names are clear.
• Code is both readable and understandable.
• The code should be reorganized to have header and .cpp files for the matrix-matrix and matrix-vector multiplication. These functions can be brought in using #include statements as opposed to redefining the functions in each .cpp file.

Can raw loops be replaced with standard library algorithms?

• No.

General questions or concerns.

• It would be nice if a Makefile or CMake build was included to compile the code.
• A std::cout << “\n”; should be added to main() in mat_vec.cpp such that the output is not printed on the same line as the terminal prompt.
• Functions above main() should be broken into independent header and .cpp files.
• Without creating header / .cpp files for the operations the code could not be used as part of a larger project.

Does the code compile?

• Yes.

Are there any compiler warnings?

• No.

Are there any runtime errors?

• No.

Does the code do what it is supposed to do?

• The code does what it is supposed to do with the exception of the saveMMfile() function, which prints the wrong indices (indices start from 0 when they should start from 1).
• The current code could not be used in any larger projects without reorganizing into a combination of header and .cpp files.

Are there test cases for all new functionality?

• Test cases do exist for all functionality but require manual comparison of the result to the expected result which is undesirable.
• Since the functions are not included from a header file, each test at the bottom of each .cpp file technically uses a different function, which is undesirable.

Are there test cases that you think should be added?

• No, but the file structure should be changed such that the same mat_mat() and mat_vec() function is tested and used in all .cpp files (create a separate header and .cpp file that have these functions. Currently there are two definitions which are the same, but do not allow for any modular or library functionality.
• Testing functions (that return either an int 0/1 or bool true/false) for a given test should be added such that the results of the tests don’t have to be manually evaluated.

Do all of the tests pass?

• All tests pass visual inspection with the exception of the saveMMfile() and saveMMfile_vector() functions which output the wrong indices (start from 0 as opposed to 1) in the Matrix Market file format.

Are there any performance regressions?

• N/A. Performance regressions were not required for this assignment.

Does the code scale to the expected problem size?

• N/A. I do not believe that this assignment has an “expected problem size”. More information would have to be given in the assignment.

[jacobmerson]

normally we do not commit .vscode files because they are system dependent. Let's add this to the .gitignore.

[jacobmerson]

Code looks great! Thanks for letting us review in class.

[jacobmerson]

I noticed that the first function was using snake_case? Do you have a style guide? We should make this consistent.

[jacobmerson]

consider using auto& for range based for loop.

[Bibek-ko-git]

Changing the variables in this file and following the provided instruction does not change the result in the result.mm file. Maybe the instruction is not clear for that.

[Bibek-ko-git]

I think the .vscode file is hindering my vscode to understand what the provided executable is. Removing that might helpful for anyone else trying to run the codes and execute in VScode. The codes and executables run fine in the ubuntu system.

The overall code is consistent and easy to follow.

Adding header files and CMakeList files would help the compilation process.

[jacobmerson]

Few major issues that will need to be resolved before you merge your code. Another one not noted in the review comments is that none of your functions have documentation.

[jacobmerson]

Consider using an alias type to avoid writing vector<vector<double>> everywhere. i.e., using Matrix = std::vector<std::vector<double>>

It is fine for this assignment, however vector<vector>> will not tend to give good performance since each column is an independent allocation.

[jacobmerson]

make sure you are not commiting any binaries. You may need to add them to your .gitignore

[jacobmerson]

The assignment asks you to implement a separate driver file and library that contains the functions to compute the two inner products. This needs to be updated. Having instructions to build these on the command line rather than CMake or a makefile is fine for assignment 0.

[jacobmerson]

The naming of this function is not ideal. If you did not have the context of the assignment, you would not know what this function does without reading the code. Consider renaming it to matrix_vector_product, or inner_product, the latter you would use the same name for matrix-vector and matrix-matrix product but take advantage of function overloading.

[jacobmerson]

Screens are wide these days, I would suggest making your name more explanatory. Without context the user would have no idea that MM means matrix_market.

[jacobmerson]

This file contains a repeat of the functions implemented in the other files which needs to be fixed for this assignment. Please make sure to update to use a separate library. Any time you ever find yourself copy-pasting code your hackles should be raised as it points to a poor choice of abstration.