MRG FIX: order of values of self.quantiles_ in QuantileTransformer

[tirthasheshpatel]

Fixes#15733

When the value of n_quantiles is too large, quantization is too dense which leads to possibility of misordering of values in self.quantiles_.
A quick fix as suggested in #15733 is implemented in this pr.

[NicolasHug]

Thanks for the PR @tirthasheshpatel, looks good .

Ideally we would need a non-regression test. Any chance we can have an easier reproducible example of the original issue?

[tirthasheshpatel]

@NicolasHug
I am not sure if the issue can be reproduced easily but I will try with some toy examples...

[glemaitre]

I think that we should create a nanpercentile and percentile in fixes.py and implement this fix for all algorithm calling using nanpercentile and percentile. I assume that other algorithms could be silently affected by this issue (e.g. KBinsDiscretizer)

@NicolasHug WDYT?

[NicolasHug]

I agree it'd be nice to have a uniform fix.

I'd suggest that we open an issue on numpy, and see what their answer/solution is. They might come with a simple or better fix than np.max.accumulate, which we could then backport. This way we're sure that we stay compatible with numpy. If their fix is too complex then we can go with the current solution.

[ogrisel]

I agree but I would also note that I find the solution based on np.maximum.accumulate quite elegant :)

[NicolasHug]

@glemaitre can you open the issue on numpy and cc me please ;)

else I'll do it soonish

[glemaitre]

@NicolasHug I will do it in the coming hours :)

[glemaitre]

I am thinking a bit more about it. np.percentile does not expect q to be an ordered sequence meaning that the np.maximum.accumulate is working for us but it will not necessarily work for numpy.

[glemaitre]

FYI The current issue was already reported in NumPy: numpy/numpy#14685

[glemaitre]

So I would be in favour to merge the fix. @tirthasheshpatel could you add an entry in what's new.
I think that we can add this bug fix in 0.22.1 as well. Could you add an entry in what's new for the 0.22.1 upcoming release.

[jnothman]

It would still be nice to add a test... But this lgtm

[NicolasHug]

LGTM

The what's new in 0.22, so that means we need to add this to the bugfix release
CC @adrinjalali

[adrinjalali]

The reproducing example in the issue itself can be a test. I think we should have it here as a test.

import numpy as np from sklearn.preprocessing import QuantileTransformer X = np.loadtxt('gistfile1.txt', delimiter=',') quantile_transformer = QuantileTransformer(n_quantiles=150).fit(X) print(np.all(np.diff(quantile_transformer.quantiles_, axis=0) >= 0)) 

[NicolasHug]

That would require copying a 300 lines data file though. We usually avoid that

[glemaitre]

We might be able to have such an example by ensuring that we have a quantile array as the miniumum example in numpy/numpy#14685

[ogrisel]

I addressed my own comment and added the missing non-regression test. If CI is green, I will merge.

[ogrisel]

I should have resolved the merge conflict with master. BTW I made sure that the test was failing on master.

[glemaitre]

Shoot, I try to produce the failure running differnent size and way of generating unsuccessfully.

[glemaitre]

Nice that you got one

[ogrisel]

The trick was to make that dataset larger than 100 samples, otherwise quantiles_ was limited by X.shape[0].

So I just duplicated the samples 10 times and the monotonicity issue was fortunately still present :)

[ogrisel]

BTW, I am not sure if quantile_transformer.quantiles_.shape[0] being smaller than quantile_transformer.n_quantiles (when the training set size is too small) is a bug or not.

But that's unrelated to the topic of this PR.

[glemaitre]

We raise warning in this case so I think that this is fine (at least we expected it)

[glemaitre]

LGTM