[ML] Parallelise the feature importance calculation for classification and regression over trees #1277
Conversation
tveasey
changed the title
| core::parallel_for_each(m_Forest->begin(), m_Forest->size(), computeTreeShap); | ||
|
|
||
| m_ReducedShapValues = m_PerThreadShapValues[0]; | ||
| for (std::size_t i = 1; i < m_PerThreadShapValues.size(); ++i) { |
There was a problem hiding this comment.
Maybe add a note here that it can be replaces with std::reduce and parallel execution once we have C++17 support?
There was a problem hiding this comment.
I don't think this can go until the standard introduces executors, because we definitely want these to be executed in a thread pool rather than have to spawn threads. So this will have to stay until we are able to adopt C++23 based on current plans. Given how far away this is, I'm not sure it is worth a comment at the moment.
| core::CContainerPrinter::print(indices)); | ||
| BOOST_REQUIRE_EQUAL(core::CContainerPrinter::print(expectedNames), | ||
| core::CContainerPrinter::print(names)); | ||
| BOOST_REQUIRE_EQUAL(core::CContainerPrinter::print(expectedShap), |
There was a problem hiding this comment.
Why is it necessary to compare the string representatives instead of the numerical values?
There was a problem hiding this comment.
It was just for code brevity, i.e. it allows me to compare all values (to reasonable accuracy) in one line.
|
Thanks for the review @valeriy42! Note I made a somewhat significant refactor in |
…n and regression over trees (elastic#1277)
We can split the work to compute feature importance up over the trees in the forest since we just sum up the per tree feature importances at the end.
In order to do this, I needed to expose an interface to
parallel_for_eachwhich takes a list of functions to run on subsets of the iterator range because we use member variables for the algorithm state to avoid reallocating for each training example, which I need to bind to each function upfront.