{"action":"created","issue":{"url":"https://api.github.com/repos/biolab/orange3/issues/1443","repository_url":"https://api.github.com/repos/biolab/orange3","labels_url":"https://api.github.com/repos/biolab/orange3/issues/1443/labels{/name}","comments_url":"https://api.github.com/repos/biolab/orange3/issues/1443/comments","events_url":"https://api.github.com/repos/biolab/orange3/issues/1443/events","html_url":"https://github.com/biolab/orange3/pull/1443","id":165030548,"number":1443,"title":"Add own implementation of tree learner","user":{"login":"janezd","id":2387315,"avatar_url":"https://avatars.githubusercontent.com/u/2387315?v=3","gravatar_id":"","url":"https://api.github.com/users/janezd","html_url":"https://github.com/janezd","followers_url":"https://api.github.com/users/janezd/followers","following_url":"https://api.github.com/users/janezd/following{/other_user}","gists_url":"https://api.github.com/users/janezd/gists{/gist_id}","starred_url":"https://api.github.com/users/janezd/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/janezd/subscriptions","organizations_url":"https://api.github.com/users/janezd/orgs","repos_url":"https://api.github.com/users/janezd/repos","events_url":"https://api.github.com/users/janezd/events{/privacy}","received_events_url":"https://api.github.com/users/janezd/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":11,"created_at":"2016-07-12T09:16:42Z","updated_at":"2016-10-07T20:08:18Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/biolab/orange3/pulls/1443","html_url":"https://github.com/biolab/orange3/pull/1443","diff_url":"https://github.com/biolab/orange3/pull/1443.diff","patch_url":"https://github.com/biolab/orange3/pull/1443.patch"},"body":"We need our own implementation of tree learner. The emphasis is not on speed (we have simple tree learner for this) and on accuracy (where SKL may (or may not) be better) but on interpretability.\r\n\r\nScikit's tree does not properly handle nominal attributes, and simple tree learner lacks features that would slow it down. This PR (WIP!) has an implementation that is rather slow, in particular for numeric attributes, but will be sped up with a grain of cython.\r\n\r\n**Note**: I believe that `Learner.fit` and `Learner.fit_storage` were not implemented as intended. Why would we force all learners to implement `fit`? Wasn't `fit` meant as a way to implement a learner without going through Orange's `Table`? But if somebody prefers `Table`, (s)he can use it. In particular, the new tree learner needs the domain, hence it cannot implement `fit` - except by wrapping the three matrices back into `Table` by using `self.domain`.\r\n\r\nBesides, condition `if type(self).fit is Learner.fit` was obviously always `False`."},"comment":{"url":"https://api.github.com/repos/biolab/orange3/issues/comments/252349157","html_url":"https://github.com/biolab/orange3/pull/1443#issuecomment-252349157","issue_url":"https://api.github.com/repos/biolab/orange3/issues/1443","id":252349157,"user":{"login":"janezd","id":2387315,"avatar_url":"https://avatars.githubusercontent.com/u/2387315?v=3","gravatar_id":"","url":"https://api.github.com/users/janezd","html_url":"https://github.com/janezd","followers_url":"https://api.github.com/users/janezd/followers","following_url":"https://api.github.com/users/janezd/following{/other_user}","gists_url":"https://api.github.com/users/janezd/gists{/gist_id}","starred_url":"https://api.github.com/users/janezd/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/janezd/subscriptions","organizations_url":"https://api.github.com/users/janezd/orgs","repos_url":"https://api.github.com/users/janezd/repos","events_url":"https://api.github.com/users/janezd/events{/privacy}","received_events_url":"https://api.github.com/users/janezd/received_events","type":"User","site_admin":false},"created_at":"2016-10-07T20:08:18Z","updated_at":"2016-10-07T20:08:18Z","body":"I consider this PR done.\r\n\r\nI understand that this will not make it into the next release. But unless there are any reasonable objections (I expect there may be some, but this time I will ignore the unreasonable ones) ... Well, I won't merge my own PR, but I'll ask @BlazZupan to do it the minute after the release.\r\n\r\nThese trees are waaaaay better than shitkit trees: they properly handle nominal attributes and unknown values. That is, unless you consider *gender=male<0.5* a proper synonym for *female*. Given all I list above, it was totally stupid that this was not merged before the workshops we had in September.\r\n\r\nClassification accuracy / MSE is the same (+- random) as that of sh-learn.\r\n\r\nRegression and classification trees are united; although there are two widgets, one is derived from the other and they are almost the same. The viewer is the same for both, too.\r\n\r\nThe test coverage is 99.51%; one of the two missing lines is related to SKL and one is \"unreachable\".\r\n\r\n- For tree.py, pylint zero issues in tree.py, and radon reports no method with complexity C, and a few B's.\r\n- For owclassificationtree, pylint reports a single issue (too many ancestors?!), and radon is perfect.\r\n- owtreeviewer has zero issues and only three B's in radon.\r\n- owtreeviewer2d has 2 issues and only one B.\r\n\r\nOverall, the changes reduces the number of issues from 268 to 146 (-122), and cyclomatic complexity from 209 to 198 (-11).\r\n\r\nI jumped through all the *reasonable* hoops I was asked for. Note also that the PR is neatly divided into no less than 19 commits. (I can split a few of them further if needed. Just ask, I now know how to do it.) TreeModel, which we proposed before I started this work, is implemented in early commits but later removed. I didn't squash this just in case.\r\n\r\n@kernc discovered that induction of a tree on the entire adult takes 5s. While this is true, nowadays trees are not used because they are fast but because they are interpretable. The trees in this PR are interpretable, while sh-learn's are fast. Who wins? (Random forest is another issue; there we use sh-learn. (For now.))\r\n\r\nFurthermore, I must stress that *it has been suggested that there will be nothing wrong with pandas slowing down most widgets by a factor of three*, with no obvious great advantages beyond following the pandas-hype. The fact that trees are going to be slower (by four seconds on a large data set resulting in a tree with 8000 nodes) is, on the other hand, inconsequential, while these trees have obvious advantages over the existing sh-kit's *gender=male<0.5* -trees.\r\n\r\nIf this all sounds like I'm still pissed about something ... this may indeed be a safe *assertion*. I can even write a short test for this."}}
{"actor":{"display_login":"janezd"}}