Hello@EvgeniDubov! Thanks for updating this PR. We checked the lines you've touched forPEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2019-12-23 09:29:09 UTC

Codecov Report

Merging#437 intomaster willnot change coverage.
The diff coverage isn/a.

@@           Coverage Diff           @@##           master     #437   +/-   ##=======================================  Coverage   98.83%   98.83%           =======================================  Files          86       86             Lines        5317     5317           =======================================  Hits         5255     5255             Misses         62       62

Continue to review full report at Codecov.

Legend -Click here to learn more
Δ = absolute <relative> (impact),ø = not affected,? = missing data
Powered byCodecov. Last updatefc9e483...7acac1f. Read thecomment docs.

Cool. I am looking forward for this contribution. Could you make a quick todo list in the first summary.

From what I see:

• We did not have any Cython up to now. So you will need to setup the project for it. You can refer tohttps://github.com/jakevdp/cython_template
• You have to solve the issues raised by PEP8.
• I think that we need to improve the example with some narrative documentation.
• We need to have a section in the User Guide such that people can find out about this feature and in which case to use it. Probably it could be embedded in the same section than the BalancedBaggingClassifier.
• You can add a what's new entry as well.

I've pushed the code for cython build support, and all the automatic checks failed.
LGTM failed because my implementation assumes the existence of sklearn/tree/_criterion.pxd and it is missing.
AppVeyor and Travis failed because Cython package is not installed.
I assume that the last will fail on the missing .pxd file too after Cython dependency will be resolved.

Please let me know if there is a way for me to configure these tools or are they administrated by the maintainers only.

@EvgeniDubov I am getting some time to look at this PR.
I will probably make a force push to have a different building system for the Cython but this is not the most important thing.

I was looking at the literature and the original paper. I did not find a clear statement on how to compute the distance in a multiclass problem which is actually supported by the trees in scikit-learn.

@EvgeniDubov @DrEhrfurchtgebietend do you have a reference for multiclass?

I have not done much multi-class classification. I do not even know how it is implemented with the traditional split criterion. Is it possible to set this up to only work for binary classification? Can we release without solving this?

@glemaitre ineed sklearn's 'gini' and 'entropy' support multiclass but hellinger requires some modification to support it.
Here is a quote from an abstract of apaper on this subject

In this paper we study the multi-class imbalance problem as it relates to decision trees (specifically C4.4 and HDDT), and develop a new multi-class splitting criterion. From our experiments we show that multi-class Hellinger distance decision trees, when combined with decomposition techniques, outperform C4.4.

I can contribute it as a separate Cython implementation, preferably in a separate PR.

Oh nice, I did not look at this paper but the 2009 and 2011. It seems that I missed it.

I can contribute it as a separate Cython implementation, preferably in a separate PR.

I think that we should have the multi-class criterion directly. The reason is that we don't have a mechanism for raising an error if the criterion is used for a multiclass problem. However, it seems that it is quite feasible to implement the algorithm 1 in the paper that you attached.

Regarding the cython file, could you take all the cython setup fromglemaitre@27fffea and just paste your criterion file and tests at the right location (+documentation). I prefer to be closer to those Cython setup (basically the major change is about the package naming).

I am curious if we need to do something specific for how feature importance will be calculated after this change is done. There are two questions here. First, does the standard method of the sum of improvement in the criterion really generalize to all criterion. I think the answer is yes but if so then it might not be the case that this is really the definition we want. In an imbalanced case we would in theory have imbalanced features (ie nearly all the same value) which if important would be used high in the tree but not frequently. This would result in a low weight under the current definition. Would a definition of the average gain when used instead of the total gain across all uses be better? To limit discussion here I put this into a SOpost.

There is 3 points to consider:

• The feature importance at a node is normalized by the weighted number of samples at that node.
• The information gain on the top of the tree will be more important than on the bottom of the tree. So a feature used on the top of the tree might be more important than a feature used several time at the bottom of the tree, if the information gain is actually lower.
• The feature importance computed in the tree is a bias estimator:http://explained.ai/rf-importance/index.html and there is probably nothing to do about that apart of doing a permutation test.

Thanks for the feedback@glemaitre .

So you agree that different split criteria could be used to calculate the feature importance in general? It seems to intuitively make sense that if it is used to build the tree it makes sense to use it to define importance.

The weighting of the importance by the number of samples at the node was sort of what got me thinking down this path. Hellinger distance is designed to be less sensitive to number of samples but I think that is only a factor in finding the split.

The permutation feature importance is a great method. I see that there arediscussions to move it to sklearn.

The purpose of thinking of feature importance in this way is to make sure one does not eliminate features which are unimportant in general but crucial in a few rare outlier cases. When doing feature selection is it easy to justify dropping such features when looking at aggregate metrics like RMSE since changes to only a few predictions will only alter it by a tiny amount. Permutation feature importance would not be sensitive to this either. Or at least it will only be as sensitive as metric you use for evaluation is to such outliers. Do you know of any standard metric for identifying features of this type? Sorry this has gotten a little off topic.

@glemaitre@chkoar I've synced with master and got lint, travis and appveyor issues, none of which caused by my contribution
can you please take a look

@glemaitre@chkoar My DS team is using Hellinger distance split criterion from@EvgeniDubov private repo. We would appreciate it being part of scikit-learn-contrib. We're willing to help move this PR forward in any way possible.

@giladwa1 I am not familiar with the Hellinger distance yet but if people are willing to help to get this merged I am ok even if it works only for the binary case.

The issue in imbalanced-learn is that we will be required to code in cython and then it had a lot of burden on the wheel generation which personally I would like to avoid if possible.

That is very true.

@glemaitre@chkoar Thanks for the quick reply, I will continue the discussion in the scikit-learn PRscikit-learn/scikit-learn#16478

@glemaitre since this PR transferred could we close this PR?

pls clarify is it added to main code or not?