[ML] Assorted runtime optimisations for classification and regression #863
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
droberts195
reviewed
Nov 29, 2019
lib/core/CDataFrame.cc
Outdated
| std::size_t beginRows, | ||
| std::size_t endRows, | ||
| TRowFunc func, | ||
| TRowFunc& func, |
There was a problem hiding this comment.
It seems dodgy to pass this by lvalue reference here, as when it's passed on again it's passed on with std::move(func). Should it be an rvalue reference here too?
Same for sequentialApplyToAllRows.
There was a problem hiding this comment.
This can be an rvalue reference, but not in sequentialApplyToAllRows because I don't move from there and pass down by reference to another function in a loop.
(It ought to be equivalent passing by lvalue reference because the upshot is it'll still get moved into SCallableWithBoundState, but perhaps passing by rvalue reference makes the intention clearer.)
tveasey
added a commit
to tveasey/ml-cpp-1
that referenced
this pull request
Dec 2, 2019
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The principal change is to bucket the candidate split points so we only check a subset of the splits when searching for the derivatives to update when computing split statistics. There are also some smaller optimisations which profiling showed up as worthwhile. The impact is largest for the case we have many metric valued features. These are pure runtime optimisations, i.e. the results are unaffected.