Skip to content

Conversation

@satyarth934
Copy link

Implementation of Multiclass RandomForest.

@phsft-bot
Copy link

Can one of the admins verify this patch?

@satyarth934
Copy link
Author

May I please know as to what kinds of tests are these? It works fine on my machine.

@vgvassilev
Copy link
Member

vgvassilev commented Mar 19, 2017

If you click on "Details" you will see. You should follow ROOT coding convention.

@satyarth934
Copy link
Author

satyarth934 commented Mar 20, 2017

Hi @vgvassilev ,
Following are the kinds of additional lines that I see added to the original code when I clicked Details.

diff --git a/tmva/pymva/src/MethodPyRFOneVsRest.cxx b/tmva/pymva/src/MethodPyRFOneVsRest.cxx
index b1113d1..9b41b39 100644
--- a/tmva/pymva/src/MethodPyRFOneVsRest.cxx
+++ b/tmva/pymva/src/MethodPyRFOneVsRest.cxx
@@ -16,7 +16,7 @@

Similar lines are added all over.
Rest of the code looks more like an output of git diff.

I am unable to understand, how to interpret these additional added lines and how to understand my mistakes.

@vgvassilev
Copy link
Member

@phsft-bot build!

@satyarth934, it seems you found your way around this. The idea of the diff is that you can apply it without having clang format. You just open the log file in plain text copy the diff, store it on your machine and apply it with git apply.

@satyarth934
Copy link
Author

@vgvassilev , Thanks.
Looks fine now.

@lmoneta lmoneta requested a review from omazapa April 13, 2017 09:09
@omazapa
Copy link
Contributor

omazapa commented Apr 14, 2017

Dear @satyarth934
This implementation is not under the architecture and design of TMVA, you dont need to create a new class to add support for multiclass in RandomForest for PyMVA.
In the same class MethodPyRandomForest we must support regression, two class classification, multiclass-classification and variable ranking.
You can see how @stwunsch give support for multiclass-classification in AdaBoost by example
a35fdf6
and may you want to rewrite you code according to TMVA design.

Cheers
Omar.

@omazapa omazapa closed this Apr 14, 2017
Copy link
Contributor

@omazapa omazapa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi,
The code dont have the required to be merged, I leave the comments for the @satyarth934
Cheers,
Omar.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants