Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[PyMVA] Refactoring of sklearn classifier with multi-class capability, unit-testing and ranking #343

Merged
merged 9 commits into from
Apr 12, 2017

Conversation

stwunsch
Copy link
Contributor

@stwunsch stwunsch commented Feb 14, 2017

This is a rebase and combination of the PRs #303 #314 #315.

  • Refactor PyRandomForest, PyGTB, PyAdaBoost
  • Add variable ranking to these classifiers
  • Add unit-tests for the sklearn classifiers
  • Add unit-test for PyKeras multiclass classification
  • Move GetMvaValues implementation from base class to method to stay independent from sklearn

You can test the changes with ctest -V -R PyMVA.

@phsft-bot
Copy link
Collaborator

Can one of the admins verify this patch?

//////////////////////////////////////////////////////////////////////////

#ifndef ROOT_TMVA_PyMethodBase
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you remove these redundant header guards? We just removed all of them from the current repo in #341

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What is the current coding style? Moving all the guards into the headers? What about using #pragma once? That would be nice ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll fix it later!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure about all the details of the coding style, but at least here I think it should be normal header guards in the respective header and no "double guards"around the include :)

@pcanal
Copy link
Member

pcanal commented Feb 15, 2017

at least here I think it should be normal header guards in the respective header and no "double guards"around the include :)

You are correct. We just changed the coding convention to not longer require the 'double guard' (which is nowadays redundant, in the past it saved in disk access).

Cheers,
Philippe.

@martinmine
Copy link
Contributor

@phsft-bot build!

@phsft-bot
Copy link
Collaborator

Can one of the admins verify this patch?

@lmoneta
Copy link
Member

lmoneta commented Apr 12, 2017

Hi Stefan,

I would like to merge this PR. Can you please verify the shown conflict with the master branch ?
Thank you
Lorenzo

@stwunsch
Copy link
Contributor Author

I've rebased and solved the conflict!

@lmoneta lmoneta merged commit 677a171 into root-project:master Apr 12, 2017
@lmoneta
Copy link
Member

lmoneta commented Apr 12, 2017

Thank you. It is now merged

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.

6 participants