{"action":"created","issue":{"url":"https://api.github.com/repos/apache/spark/issues/14547","repository_url":"https://api.github.com/repos/apache/spark","labels_url":"https://api.github.com/repos/apache/spark/issues/14547/labels{/name}","comments_url":"https://api.github.com/repos/apache/spark/issues/14547/comments","events_url":"https://api.github.com/repos/apache/spark/issues/14547/events","html_url":"https://github.com/apache/spark/pull/14547","id":170038582,"number":14547,"title":"[SPARK-16718][MLlib] gbm-style treeboost","user":{"login":"vlad17","id":5834964,"avatar_url":"https://avatars.githubusercontent.com/u/5834964?v=3","gravatar_id":"","url":"https://api.github.com/users/vlad17","html_url":"https://github.com/vlad17","followers_url":"https://api.github.com/users/vlad17/followers","following_url":"https://api.github.com/users/vlad17/following{/other_user}","gists_url":"https://api.github.com/users/vlad17/gists{/gist_id}","starred_url":"https://api.github.com/users/vlad17/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/vlad17/subscriptions","organizations_url":"https://api.github.com/users/vlad17/orgs","repos_url":"https://api.github.com/users/vlad17/repos","events_url":"https://api.github.com/users/vlad17/events{/privacy}","received_events_url":"https://api.github.com/users/vlad17/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":16,"created_at":"2016-08-08T22:14:59Z","updated_at":"2016-08-19T23:30:57Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/apache/spark/pulls/14547","html_url":"https://github.com/apache/spark/pull/14547","diff_url":"https://github.com/apache/spark/pull/14547.diff","patch_url":"https://github.com/apache/spark/pull/14547.patch"},"body":"## What changes were proposed in this pull request?\r\n\r\n#### TreeBoost\r\nThis change adds TreeBoost functionality to `GBTClassifer` and `GBTRegressor`. The main change is that leaf nodes now make a prediction which optimizes the loss function, rather than always using the mean label (which is only optimal in the case of variance-based impurity).\r\n\r\n#### Changes: L2 and Logistic loss-based impurities\r\n\r\nThis changes the defaults to use the loss-based impurity rather than the required variance.\r\n\r\nI made this change only for L2 loss and logistic loss (adding some aliases to the names as well for parity with R's implementation, GBM). These two functions have leaf predictions that can be computed within the framework of the current impurity API. Other loss functions will require API modification, which should be its own change, SPARK-16728.\r\n\r\nNote that because loss-based impurity with L1 loss is NOT supported, the default behavior for the GBTRegressor is to use the variance-based impurity (since the aformentioned combination throws).\r\n\r\n## How was this patch tested?\r\n\r\n#### Correctness\r\n\r\nI tested defaults parameter values and new settings for the parameters with new unit tests.\r\n\r\n#### Accuracy\r\n\r\nExample prediction problem with [UCI half of million-song dataset](http://archive.ics.uci.edu/ml/datasets/YearPredictionMSD). \r\n\r\nThis code is a pretty aesthetic change: the only algorithm that differed is the logistic loss one, and the accuracy is identical to that of variance. The difference could only visible in the raw prediction (and thus AUC), since leaf predictions for minimal logistic loss and the mean produced equivalent outcomes after thresholding.\r\n\r\nWith 700 trees and 0.001 shrinkage, runtime is nearly equivalent (within 2%) [script and output here](https://gist.github.com/vlad17/c22bb4e6679c9e65fcc3e93a92bd3c30).\r\n\r\n[GBM run for comparison](https://gist.github.com/vlad17/eb71cdbcc5e4f1ad891e482a84c8d82b) Everything run on my machine\r\n\r\n\r\n"},"comment":{"url":"https://api.github.com/repos/apache/spark/issues/comments/241159859","html_url":"https://github.com/apache/spark/pull/14547#issuecomment-241159859","issue_url":"https://api.github.com/repos/apache/spark/issues/14547","id":241159859,"user":{"login":"vlad17","id":5834964,"avatar_url":"https://avatars.githubusercontent.com/u/5834964?v=3","gravatar_id":"","url":"https://api.github.com/users/vlad17","html_url":"https://github.com/vlad17","followers_url":"https://api.github.com/users/vlad17/followers","following_url":"https://api.github.com/users/vlad17/following{/other_user}","gists_url":"https://api.github.com/users/vlad17/gists{/gist_id}","starred_url":"https://api.github.com/users/vlad17/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/vlad17/subscriptions","organizations_url":"https://api.github.com/users/vlad17/orgs","repos_url":"https://api.github.com/users/vlad17/repos","events_url":"https://api.github.com/users/vlad17/events{/privacy}","received_events_url":"https://api.github.com/users/vlad17/received_events","type":"User","site_admin":false},"created_at":"2016-08-19T23:30:57Z","updated_at":"2016-08-19T23:30:57Z","body":"@sethah Was that coupling not already there beforehand? I didn't really change any of the implementation class' interfaces, I just added the Bernoulli impurity to the existing Impurity framework, which itself couples the Impurity class with the ImpurityAggregator, that necessarily returns an ImpurityCalculator, which makes the prediction. It seems like the existing design is already doing the coupling.\r\n\r\nAs for the interface making this coupling explicit, yes, I completely agree I'm doing that. But I think this is a good thing.\r\n1. The coupled loss functions / splitting impurity is the whole point of tree boost. The papers themselves say to construct intermediate trees to minimize loss. They only offer using other impurity measures for ease of implementation. XGBoost, for instance, splits on (an approximation of) the losses directly.\r\n2. The fact that the underlying impurity/predictions are all done by the same class (though not my choice), is also probably better from an implementation perspective. Both need to gather summary statistics about each leaf's partition of the data, so it's easiest to just do it in one place.\r\n3. I don't think we're giving up the \"Decoupled version\" either. If we so choose to in the future, setting impurity to \"variance\" but loss function to \"absolute\" can use a new ImpurityAggregator that offers the variance for splitting but median for predicting.\r\n\r\nMy goal with this PR was to make as minimal a change as possible (it's mostly an API change introducing the loss-based impurity, which also makes loss-based terminal node predictions). I'm not trying to change the GBT design here at all (though if it appears to be the case because of something I'm missing, please let me know).\r\n\r\n"}}
{"actor":{"display_login":"vlad17"}}