Overload init(). Issue #61#73
Conversation
…and std::vector<std::string>
…and std::vector<double>
There was a problem hiding this comment.
There are just two problems, which you need to resolve. Rest you have done a great job just add at least one unit test for each method in tests/initialization_tests.hpp. It will help us avoid any bad code to enter the library and keep it stable. So, you will have to write at least 4 unit tests as you have worked on 4 new methods.
After writing the appropriate tests run it using mkdir build/tests && cd tests && g++ tests.cpp -I../include -lgtest -lpthread -o ../build/tests/tests && ../build/tests/tests
If you have any other questions feel free to ask them.
| /// Method to initialize values of a Matrix object using a string | ||
| Matrix MatrixOp::init(std::string s) { | ||
| Matrix result; | ||
| std::vector<std::vector<std::string>> * vec_s = new std::vector<std::vector<std::string>>(1); |
There was a problem hiding this comment.
You have dynamically allocated memory using new keyword. This will result in memory leakage as the memory will not be released explicitly by the compiler when the scope of the variable reaches an end. It would be better if you take advantage of the dynamic nature of std::vector to your advantage instead.
There was a problem hiding this comment.
@aniketsharma00411 I have addressed all suggestions here in my latest commit. I was unable to run the unit tests due to an error. There seems to be no 'gtest/gtest.h' file anywhere that I can find, yet each test .hpp includes it at the top. I looked for a 'test/test.h' file as well but could not find it anywhere. Does this file exist in the current repository? I'm not able to compile and run my tests if it does not
There was a problem hiding this comment.
Run git submodule update --init and then you will be able to run unit tests.
Refer to #69 if you want to delve deeper into the problem but it isn't necessary.
There was a problem hiding this comment.
Also, delete lines 114 and 124 as suggested by CodeFactor.
There was a problem hiding this comment.
@aniketsharma00411 okay I addressed the quotation and whitespace issue. Those commits have been pushed. I'm still having issues compiling to test, however. I tried 'git submodule update --init' and then compiling with 'g++ tests.cpp -I../include -lgtest -lpthread -o ../build/tests/tests' but I still get that gtest is missing. I tried including ../include and ../lib/googletest/googletest/include, which attempted to compile, but then I got more errors for the method Matrix::genfromtxt. I'm sorry I'm very unfamiliar with compiling with libraries, even more so in the case of submodules and git. Is there any advice you can give me for the future for other work I might want to do for this project?
There was a problem hiding this comment.
This might be an installation issue of googletest. Try running the following in lib/googletest/ directory
mkdir build
cd build
cmake ..
And then try compiling again. Let me know if it works or not.
There was a problem hiding this comment.
I installed cmake and tried running those commands but it gave me an error:
-- The C compiler identification is unknown
-- The CXX compiler identification is unknown
CMake Error at CMakeLists.txt:10 (project):
The CMAKE_C_COMPILER:
cl
is not a full path and was not found in the PATH.
There was a problem hiding this comment.
Which OS are you using?
There was a problem hiding this comment.
This problem will be resolved when #76 is merged. Try it after that.
|
|
||
| Matrix mat = matrix.init(vec); | ||
|
|
||
| Matrix test_with = matrix.genfromtxt('test_dataset.csv', ','); |
There was a problem hiding this comment.
At lines 92, 106, 117 and 127, 'test_dataset.csv' should be "test_dataset.csv". Just correct that and then I will merge this PR.
I have checked everything else and it works fine.
aniketsharma00411
left a comment
aniketsharma00411
left a comment
There was a problem hiding this comment.
All unit tests have passed. I will be merging this PR.
2 participants
Description
Overloads init() method for double, std::string, std::vectorstd::string and std::vector. The overloaded functions mirror already existing ones but provide the functionality to create matrices with 1 row. The keyword new is used in order to allocate sufficient memory from within the function.
Fixes # 61
Type of change
Please delete options that are not relevant.
Checklist: