Quantile encoder

[cmougan]

This PR (#302), implements two methods from a recently published paper at a conference (MDAI 2021).

Quantile Encoder: Tackling High Cardinality Categorical Features in Regression Problems (Carlos Mougan, David Masip, Jordi Nin, Oriol Pujol)

Encoding methods, full technical development can be followed in the paper:

• Quantile Encoder

Tests are implemented and passed.
Scikit learn API semantics
Docs is extended

If I missed something or any comments, please let me know :)

[cmougan]

@janmotl @wdm0006

[PaulWestenthanner]

Hi @cmougan,
Sorry for the late reply. I've read trough the paper today. Great piece of work!
I've noticed you've pushed the code to sktools in the meantime (which also uses this repo). Is this PR still relevant? Would you prefer to have the code merged into here and remove it from sktools?

[cmougan]

Hi @PaulWestenthanner

Many thanks for your answer.

We will be very happy to contribute to your package. It makes all the sense to have it as another method in category_encoders.

So, yes please, let's merge.

The only reason to keep it alive in stools it's because the congress proceedings paper points there. Still we will like to merge.

Let us know what we can do.

[PaulWestenthanner]

Hi @cmougan
I just reviewed your code and remarked some minor changes - many of them rather cosmetic. Overall the code is quite nice and will be a good improvement to the library.
Apart from the comments I'd like to point out that I'm not quite happy with to copy-paste of some functionality (e.g. dropping low-variance columns). However I think it's okay here to just copy-paste since most other encoders did the same and we'll need a separate issue to clean it up.
Regarding the SummaryEncoder: if you want to you can move it over here as well.

[cmougan]

@david26694

[cmougan]

Many thanks @PaulWestenthanner for the detailed review.

I just pushed the Summary Encoder. If I have not missed anything, everything should be there.

[PaulWestenthanner]

if you just add this in the for loop below (either before or after the col-names.append(,,,) you don't need the two loops here and don't need to calculate the percentile.
Note that the comment on coinciding quantiles below also applies here.

[PaulWestenthanner]

Thanks for the update. I've added a second round of comments.
Note that formatting the __init__.py introduced some merge conflicts. Please resolve them as well

[cmougan]

Hi @PaulWestenthanner I believe we just covered your comments. We also did a review of possible things that we might have left out.

Do you see anything else that might be worth to improve? Or that we missed out?

Many thanks for your suggestions, we really appreciate them :)

[PaulWestenthanner]

The code looks fine to me now. I'm happy to merge as soon as we get the pipeline to run. The issue at the moment is that we support python 3.5 which does not support f-strings yet. Since f-strings are awesome I'd recommend using the fstring package from future. Check this SO post as reference https://stackoverflow.com/questions/55182209/f-string-invalid-syntax-in-python-3-5
You'll probably need to add this package to the requirements(-dev).txt

[PaulWestenthanner]

Maybe we should drop support for python 3.5 since it is no longer officially supported anyway. But that would be out of scope for this PR

[cmougan]

@PaulWestenthanner thanks again!

[cmougan]

Hi @PaulWestenthanner,

After some discussion and testing with @david26694 we notice that the summary encoder does not pass most of the test.

• 1. Fix summary encoder to pass all the tests, this seems like a lot of work
• 2. Use the same strategy as with QE and copy-paste a lot of code.
• 3. Don't add Summary Encoder in this PR
• 4. Merge with out passing the tests

[PaulWestenthanner]

Hi @cmougan @david26694

Thanks for adding the tests. This was a good catch!
I took the time to fix the tests. This was mainly because the iterative structure did not comply with other encoders.
Please find my code here: https://github.com/PaulWestenthanner/categorical-encoding/tree/quantileEncoder
It's a little mix of option 1 and 2 since a little code is copied. However, the part of the code that pre-processes X and y and checks for nulls and so on is copied for more or less any encoder in the repo. This issue should be tackled at some point but not now.

I'd suggest we merge my branch into yours and then into master?

[cmougan]

Hi @PaulWestenthanner @david26694

Waw, impresive coding.

Just merged your branch.

[PaulWestenthanner]

The tests still fail for python 3.5 since f-strings are used. I fixed that in the code but forgot the tests. Would you mind fixing this @cmougan ?

[cmougan]

@PaulWestenthanner and now?

[PaulWestenthanner]

Sorry I missed one of the tests. I've added a comment on the appropriate position

[cmougan]

@PaulWestenthanner I was trying to debug that very unsuccessful. Many thanks

[PaulWestenthanner]

Great! Thanks for your effort. LGTM & Merge