decile-team / submodlib

Summarize Massive Datasets using Submodular Optimization
MIT License
88 stars 33 forks source link

General C++ Compilation Issues #4

Open nab170130 opened 3 years ago

nab170130 commented 3 years ago

While the pip installation works for Google Colab, I am still running into issues installing on a Windows system with C++20 VS build tools. There are quite a few warnings; more importantly, there are a couple errors that I've identified from the full log output (PIP Log Output.txt):

C:\Users\nbeck\AppData\Local\Temp\pip-install-d83pcjkv\submodlib_df4056be6b38418fbc6a76e7ace5d2d4\cpp\SetFunction.cpp(12) : error C4716: 'SetFunction::getEffectiveGroundSet': must return a value C:\Users\nbeck\AppData\Local\Temp\pip-install-d83pcjkv\submodlib_df4056be6b38418fbc6a76e7ace5d2d4\cpp\SetFunction.cpp(10) : error C4716: 'SetFunction::marginalGainWithMemoization': must return a value C:\Users\nbeck\AppData\Local\Temp\pip-install-d83pcjkv\submodlib_df4056be6b38418fbc6a76e7ace5d2d4\cpp\SetFunction.cpp(9) : error C4716: 'SetFunction::marginalGain': must return a value C:\Users\nbeck\AppData\Local\Temp\pip-install-d83pcjkv\submodlib_df4056be6b38418fbc6a76e7ace5d2d4\cpp\SetFunction.cpp(8) : error C4716: 'SetFunction::evaluateWithMemoization': must return a value C:\Users\nbeck\AppData\Local\Temp\pip-install-d83pcjkv\submodlib_df4056be6b38418fbc6a76e7ace5d2d4\cpp\SetFunction.cpp(7) : error C4716: 'SetFunction::evaluate': must return a value C:\Users\nbeck\AppData\Local\Temp\pip-install-d83pcjkv\submodlib_df4056be6b38418fbc6a76e7ace5d2d4\cpp\optimizers\LazierThanLazyGreedyOptimizer.cpp(49) : error C4716: 'printSortedSet': must return a value C:\Users\nbeck\AppData\Local\Temp\pip-install-d83pcjkv\submodlib_df4056be6b38418fbc6a76e7ace5d2d4\cpp\SetFunction.cpp(13) : error C4716: 'SetFunction::maximize': must return a value

All the errors revolve around the lack of a return statement in functions that have return types. As a suggestion, perhaps the SetFunction class could instead declare its functions as pure virtual functions so that they can be abstract definitions/declarations (as they do not do anything by their current definition). Furthermore, perhaps the SetFunction class could declare a blank virtual destructor as not all compilers will bind delete calls from base-class pointers to the most-derived class's destructor without this definition.

vishkaush commented 3 years ago

Sure. Can you pls go ahead and make the changes? Make sure to run all the tests after making the changes. After that you can create a pull-request so that I can review the changes and merge.

nab170130 commented 3 years ago

Sure, I can make these changes. I'll start on a new branch.