(Draft: allow disabling calculation in separate threads)

[0x7f]

Why: I'd like to integrate ranger into another application which already manages its thread pool and would like to therefore disable ranger's threading. Also spawning one or more threads per prediction call is also not an option latency-wise.

For now, this is just a draft as I'm not happy with the current state (esp. code path could be merged with the one inside the #ifdef WIN_R_BUILD) and would like to hear your feedback first.

Regarding naming: Maybe threading_enabled would be more readable in the code?

So what do you think?

[0x7f]

Sorry for the delay. I finally found some time to play around with the R binding. If I understand it correctly, you must do the processing in a separate thread, even if num_threads == 1. Because if not, you would lose the possibility to print the progress to the R console as the tree calculations block that thread. So I think i would be better to fix the issue on Windows. Will try to set up the build environment on my Windows machine.

[mnwright]

In the Windows-R-build multithreading is not (yet) supported at all. We just print progress and check for user interrupt after computation of each tree. We could use this for num_threads = 1, too.

[0x7f]

I just checked the code-path for WIN_R_BUILD again. Sorry if I get you wrong, but I do not see where it should break. Was not able yet to build it on windows, so don't know whether it compiles - cygwin is still installing ;)

num_threads is set to 1 in both initialization cases:

• before:

  ranger/source/src/Forest/Forest.cpp

  Lines 194 to 196 in 68abe22

  	#ifdef WIN_R_BUILD
  	this->num_threads = 1;
  	#else

• after: https://github.com/0x7f/ranger/blob/1c31c1765b8e6ff5562c5b46cf4cde2c6549f1ff/source/src/Forest/Forest.cpp#L203-L206

Growing trees prints status after growing each tree:

• before:

  ranger/source/src/Forest/Forest.cpp

  Lines 415 to 425 in 68abe22

  	// Grow trees in multiple threads
  	#ifdef WIN_R_BUILD
  	progress = 0;
  	clock_t start_time = clock();
  	clock_t lap_time = clock();
  	for (size_t i = 0; i < num_trees; ++i) {
  	trees[i]->grow(&variable_importance);
  	progress++;
  	showProgress("Growing trees..", start_time, lap_time);
  	}
  	#else

• after: https://github.com/0x7f/ranger/blob/1c31c1765b8e6ff5562c5b46cf4cde2c6549f1ff/source/src/Forest/Forest.cpp#L463-L474

Same for prediction (should also be the same for all remaining cases where I refactored WIN_R-handling the same way):

• before:

  ranger/source/src/Forest/Forest.cpp

  Lines 479 to 488 in 68abe22

  	#ifdef WIN_R_BUILD
  	progress = 0;
  	clock_t start_time = clock();
  	clock_t lap_time = clock();
  	for (size_t i = 0; i < num_trees; ++i) {
  	trees[i]->predict(data, false);
  	progress++;
  	showProgress("Predicting..", start_time, lap_time);
  	}
  	#else

• after: https://github.com/0x7f/ranger/blob/1c31c1765b8e6ff5562c5b46cf4cde2c6549f1ff/source/src/Forest/Forest.cpp#L513-L524

Could you please point me to the point where WIN_R_BUILD breaks?

[mnwright]

In the new version std::thread is used without #ifndef WIN_R_BUILD or similar, e.g., here:
https://github.com/0x7f/ranger/blob/1c31c1765b8e6ff5562c5b46cf4cde2c6549f1ff/source/src/Forest/Forest.cpp#L429

In the R Windows version, an old gcc is used where std::thread is not available. The C++ Windows version should work fine. Btw, I never tried to build it on Windows, I usually cross compile, scripts are here:
https://github.com/imbs-hl/ranger/tree/master/cross_compile

[0x7f]

You are absolutely right - this code path was hidden to WIN_R before due to the ifdefs. Will change that.

[codecov-io]

Current coverage is 76.62%

Merging #41 into master will not affect coverage as of eba692e

@@            master     #41   diff @@
======================================
  Files           11      11       
  Stmts          646     646       
  Branches         0       0       
  Methods          0       0       
======================================
  Hit            495     495       
  Partial          0       0       
  Missed         151     151       

Review entire Coverage Diff as of eba692e

Powered by Codecov. Updated on successful CI builds.

[0x7f]

@mnwright I fixed the mentioned issue and squashed the commits. Whats your opinion about the current state?

[vkhodygo]

Why hasn't this been merged yet?

[mnwright]

We still want this, but this PR is outdated. I'll probably have to start from scratch.