FIX validate input array-like in KDTree and BallTree

[cmarmo]

Reference Issues/PRs

Closes #14650
Superseded and closes #17400

[cmarmo]

This is failing with a segfault that I can reproduce locally with python 3.8 (not yet tested with python 3.6).
The segfault went away if I downgrade pytest to version 5.2.1, with 5.3.1 the segfault is back again. I need to investigate more, @ogrisel, @rth, @lesteve ... if you want to have a look... :)

[ogrisel]

The segfault went away if I downgrade pytest to version 5.2.1, with 5.3.1 the segfault is back again.

This is really weird. Ideally it would be great to isolate a minimal reproducer that does not depend on pytest in a simple python script to understand better what's going on.

[cmarmo]

The code was asking for some invalid properties before the validation step, quite surprised that some checks were passing before... :(

[glemaitre]

Most probably because we were passing an array but not a list of list in the tests.

[glemaitre]

Then I think that we need an additional test where we can pass an array-like and check that it works while it would have failed before. The sequence given is expected to fail.

[glemaitre]

LGTM

[ogrisel]

I am worried about the performance overhead of doing a nested check_array when BallTree / KDTree are used inside a KNearestNeigborsClassifier or similar.

In particular the check_array does a linear sequential pass over the data to detect NaNs and (+/-)inf values which will be done twice in this case. I don't know whether or not this is negligible or not in particular on multicore machines with 16+ cores. For instance on KMeans this kinds of input validation checks starts to be visible now that multicore scalability has been optimized.

But this problem is probably impacting other estimators so we might want to defer its resolution for a later PR. Just something for @jeremiedbb to keep in mind when we start to investigate the perf overhead of those input checks.

[glemaitre]

@ogrisel temporarily, we could make the check_array within a context manager set_config(assume_finite=True).

[ogrisel]

@ogrisel temporarily, we could make the check_array within a context manager set_config(assume_finite=True).

Interesting idea, but this should be done in the scikit-learn code (e.g. KNearestNeigborsClassifier or similar) that calls into BallTree / KDTree because this code knows whether the checks have already been done or not.

[cmarmo]

Interesting idea, but this should be done in the scikit-learn code (e.g. KNearestNeigborsClassifier or similar) that calls into BallTree / KDTree because this code knows whether the checks have already been done or not.

@ogrisel, am I understanding correctly saying that the context manager should not be added here? It is not clear to me if I have to change anything in this PR.

[ogrisel]

@ogrisel, am I understanding correctly saying that the context manager should not be added here? It is not clear to me if I have to change anything in this PR.

The context manager should be used to wrap the calls to BallTree from other classes in scikit-learn that have already validated there input data to avoid duplicating this check. Since this PR adds a new check, it would be great to do it as part of this PR.

[ogrisel]

@thomasjpfan did something similar in #18727 and the benchmarks show that it works as expected.

[ogrisel]

Looking again at this PR, the new input check is in the init of the BallTree. This operation is very compute intensive, so duplicating the input data check is probably not a significant performance regression.

However we have a similar problem (already in master) when KNearestNeigbhorsClassifier.predict calls BallTree.query or query_radius. In my opinion, this case is much more problematic from a performance point of view. But fixing those will extend the scope of the current PR too much.

Let's do this as part of a dedicated PR with proper benchmarking and such.

+1 for merging the current PR as it is in the mean time.

[ogrisel]

Thank you very much for the fix @cmarmo !

[cmarmo]

@ogrisel, let me check if the failure is related to this PR, then I will open an issue about the performance problem. Thanks for your review!

[ogrisel]

The test_poisson_zero_nodes failure is unrelated and already tracked in #18737. Let's merge.