Skip to content

[MRG+1] Export ClassificationCriterion and RegressionCriterion #10325

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Dec 18, 2017
Merged

[MRG+1] Export ClassificationCriterion and RegressionCriterion #10325

merged 1 commit into from
Dec 18, 2017

Conversation

camilstaps
Copy link
Contributor

Reference Issues/PRs

Resolves #10251.

What does this implement/fix? Explain your changes.

This allows extending of ClassificationCriterion and RegressionCriterion. For an example use case, see https://stats.stackexchange.com/q/316954/98500.

Any other comments?

I wasn't sure what to do with the documentation. The documentation on Criterion is different in the pyx and the pxd, and the latter is not a docstring. I copied the very succinct docstring for ClassificationCriterion and wrote a similar one for RegressionCriterion. But as Criterion does something weird, which I cannot find in the coding guidelines, I don't know what I should do there. Advise appreciated.

Because this is not a public API, I did not provide usage examples. (A usage example would by any of the subclasses defined in the pyx.) Please let me know if they should be added.

Thanks for reviewing!

@jnothman
Copy link
Member

I'm fine with this as an intermediate step. LGTM.

@jnothman jnothman changed the title [MRG] Export ClassificationCriterion and RegressionCriterion [MRG+2] Export ClassificationCriterion and RegressionCriterion Dec 18, 2017
@jnothman jnothman changed the title [MRG+2] Export ClassificationCriterion and RegressionCriterion [MRG+1] Export ClassificationCriterion and RegressionCriterion Dec 18, 2017
@glemaitre glemaitre merged commit d523243 into scikit-learn:master Dec 18, 2017
@glemaitre
Copy link
Member

LGTM. Merging ... thanks @camilstaps

@camilstaps camilstaps deleted the 10251-extend-criterion branch December 18, 2017 17:31
@rajday19
Copy link

@camilstaps
Is the extension for the regression criterion part of the main scikit-learn?

I need to create a custom criterion function for the RandomForestRegressor. Can you provide an example of how to pass a custom function using the new patch? Much appreciated.

@camilstaps
Copy link
Contributor Author

@rajday19 This PR only facilitates extensions, it didn't add any extensions itself.

I'm afraid I can't help much with writing the custom criterion, see the discussion at the end of #10251.

This was referenced Jul 10, 2018
Closed
Closed
@EvgeniDubov
Copy link

EvgeniDubov commented Oct 9, 2018 •
edited
Loading

@glemaitre
I'm working on Hellinger Distance Criterion cython implementation of ClassificationCriterion in imblearn #437
For the new criterion implementation I need to inherit the ClassificationCriterion using this line:
from sklearn.tree._criterion cimport ClassificationCriterion
In order for this line to build successfully I need _criterion.pxd file to be located in sklearn\tree
However when I install scikit-learn 0.20 the .pxd is not deployed and I get the following error during cython build:

from sklearn.tree._criterion cimport ClassificationCriterion
^
------------------------------------------------------------
imblearn\tree\criterion.pyx:10:0: 'sklearn\tree\ _criterion.pxd' not found

I went through sklearn setup files and MANIFEST and looks like the .pxd should have got into the whl and be deployed

Can you please direct me on the proper way to inherit the exported ClassificationCriterion?

Thank you

@EvgeniDubov EvgeniDubov mentioned this pull request Oct 9, 2018
Open
@glemaitre glemaitre mentioned this pull request Oct 12, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jnothman jnothman jnothman approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Extending Criterion
5 participants
@camilstaps @jnothman @glemaitre @rajday19 @EvgeniDubov