Random Forest added #2282
Random Forest added #2282
Conversation
|
@iglesias I have added RF w/o unittests. I will add unittests tomorrow. But you may start reviewing. |
|
|
||
| namespace shogun | ||
| { | ||
| class CRandomForest : public CBaggingMachine |
There was a problem hiding this comment.
Documentation is missing.
|
@vigsterkr, maybe you can have a quick look and see if the changes in BaggingMachine are all right for you. I think they are. |
|
@mazumdarparijat, see the comments above. The most important is the large amount of code duplication in the train method of CRandomCARTree ;-) Please, keep this pull request of this size. You may add unit tests, but nothing else before we merge it. So the plan is to address the comments, add unit tests and then it is ready to be merged. Good job and thanks! |
|
yes! I will just add unittest and include your comments. Thats it! :-) |
|
@iglesias I have addressed your previous comments, added unittest as well. There were also quite a few errors and leaks in BaggingMachine which I fixed. Please have a look! |
| @@ -81,7 +81,7 @@ SGVector<float64_t> CBaggingMachine::apply_get_outputs(CFeatures* data) | |||
| { | |||
| CMachine* m = dynamic_cast<CMachine*>(m_bags->get_element(i)); | |||
| CLabels* l = m->apply(data); | |||
| SGVector<float64_t> lv = l->get_values(); | |||
| SGVector<float64_t> lv = dynamic_cast<CDenseLabels*>(l)->get_labels(); | |||
There was a problem hiding this comment.
For casting labels, use obtain_from_generic please. This way the actual cast is only written in one place and it is more practical if at some time we need to modify the way we are doing the casts.
There was a problem hiding this comment.
@iglesias there is no obtain_from_generic method for CDenseLabels. I have added check for NULL pointer in my latest commit, that should solve the possible coverity defect.
|
It seems that this has broken the unit test of Bagging machine. Can you fix that, @mazumdarparijat? |
|
@iglesias I have fixed the existing unittest! I must say that though the fix is very simple, but I really got tired finding the problem. Phew! ;-) |
|
Right about obtain_from_generic in labels. We have that method for features, for labels we have the class LabelsFactory. |
|
So shouldn't the shogun feature page be updated? |
|
@jondo oh yeah, it should be. it can be updated via: would really appreciate the update. |
|
May I suggest to do a proper comparison with some other implementation, say scikit-learn? |
|
I have updated now the sheet above with decision trees in Shogun. Didn't Also, http://www.shogun-toolbox.org/page/features/ seems to be sort of On 26 September 2014 13:26, Heiko Strathmann notifications@github.com
|
Reference