Stoozy / mercury

Event Driven Backtesting Framework
MIT License
1 stars 5 forks source link

Modifying README.md #3

Open pranavjainjs opened 11 months ago

pranavjainjs commented 11 months ago

I noticed the following issues in your source code which many people will encounter while installation.

  1. You have not mentioned which OS the commands in the README are for. cmake .. gives many errors for Windows. CMakeLists.txt has to be modified to mitigate those errors.
  2. For some Linux versions, you need to link using pthread library to use pthread_create and pthread_join functions. This could be fixed just by adding a line target_link_libraries(mercury pthread) in CMakeLists.txt
  3. The README is not beginner friendly. It seems that you expected the people with some background knowledge to contribute. But having some additional information in README will help everyone. This includes -
    • Informing how to download AAPL.csv and modifying start_date and end_date in config.json
    • OS specific cmake instructions
pranavjainjs commented 11 months ago

I can help with 2 and 3.

Stoozy commented 11 months ago
  1. The whole point of using cmake is to not rely on specific compilers and OSes, so OS specific commands are not really my problem. (I've tested using MinGW on windows and gcc on linux, it works fine, MSVC may have issues that I'm not aware of, I'll deal with that another time). So just a comment about using MinGW on windows should suffice.
  2. See 1.
  3. Modifying start_date and end_date is pretty self-explanatory and even if we do, we would then have to explain every other part of the config file, not just the start_date and end_date. As for obtaining the historical data, you can download the data from yahoo finance (note: it needs to be in the same directory as the mercury executable), sorry I forgot to mention that.

Other readme issues:

Finally, thank you for the help :)

pranavjainjs commented 11 months ago
  1. It still does not work in my Windows device. I don't know why.
  2. I think you did not understand my point. When I run make (in Linux), the compiler gives error 'undefined reference to 'pthread_create''. This error occurs because the library pthread needs to be added during linking (which is done using -pthread flag in gcc instruction). Adding a line target_link_libraries(mercury pthread) in CMakeLists.txt does exactly that.
  3. Yes, you are absolutely right those fields - start_date and end_date - are self explanatory. But since you are not providing AAPL.csv file, and it is used in config.json, it is customary to tell people how to download the file and change any parameters for the new file.

I will include the other readme issues as well. Will you include this issue in hacktoberfest under no-code contributions?

Stoozy commented 11 months ago

Provide some information on the issues you have with compiling (what compiler you use, OS and other relevant info).

As for the rest of the readme, you can proceed. I'm not sure how to add it to hacktoberfest contributions, I need some info on that.

pranavjainjs commented 11 months ago

To participate in Hacktoberfest, you need to do the following things before accepting any PR. For more details, refer the website.

  1. Add hacktoberfest topic in your repository.
  2. Add hacktoberfest label in the issue.
  3. Accept PRs by approving or by adding hacktoberfest-accepted label to the PRs.

I will proceed with the README as discussed. Check #4.

As for the OS, I am using Windows 11. My compilers are MinGW gcc and g++. I was getting the following error -

-- Building for: NMake Makefiles
CMake Error at CMakeLists.txt:16 (project):
  Running

   'nmake' '-?'

  failed with:

   The system cannot find the file specified

CMake Error: CMAKE_C_COMPILER not set, after EnableLanguage
CMake Error: CMAKE_CXX_COMPILER not set, after EnableLanguage
-- Configuring incomplete, errors occurred!

I saw a similar answer on StackOverflow which requires a change in CMakeLists.txt. On a second note. Currently, I am not using Visual Studio. Are there some files missing from my system? Will the issue be resolved if I install Visual Studio?

Stoozy commented 11 months ago

You're not generating mingw makefiles, use this command: cmake .. -G "MinGW Makefiles". The error has nothing to do with visual studio if you're using mingw.

pranavjainjs commented 11 months ago

Okay. I followed your instructions (which should be added in README too). I am glad it worked and I was able to create the Makefile.

Now, I am getting the following error when I run make command.

...\mercury\src\backtest.cc:22:8: error: 'thread' is not a member of 'std'
   22 |   std::thread(&MarketSimulation::run, MarketSimulation(m_config)).detach();
      |        ^~~~~~
...\mercury\src\backtest.cc:8:1: note: 'std::thread' is defined in header '<thread>'; did you forget to '#include <thread>'?
    7 | #include <mean_reversion.h>
  +++ |+#include <thread>
    8 |
make[2]: *** [CMakeFiles\mercury.dir\build.make:76: CMakeFiles/mercury.dir/src/backtest.cc.obj] Error 1
make[1]: *** [CMakeFiles\Makefile2:82: CMakeFiles/mercury.dir/all] Error 2
make: *** [Makefile:90: all] Error 2

But, library thread is included in both files.

Stoozy commented 11 months ago

I think you may have a weird MinGW configuration. Take a look at this: https://stackoverflow.com/questions/21460817/why-do-i-get-this-error-thread-is-not-a-member-of-std