[fix] correct logic k-NN algos kneighbors() call when algorithm='brute' and fit with GPU
#2056
Conversation
icfaust
requested review from
Alexsandruss and
samir-nasibli
as code owners
|
/intelci: run |
|
onedal knn implementation is a nightmare |
|
/intelci: run |
There was a problem hiding this comment.
Nice work @icfaust.
Just a questions: could we validate the perf, since the flow is changed a little bit?
Co-authored-by: Samir Nasibli <[email protected]>
|
/intelci: run |
|
/intelci: run |
There was a problem hiding this comment.
@icfaust I am good with the changes proposed. Just minor comments
I recommend wait CI back for the last validation
| result = neigh.kneighbors(test, 2, return_distance=False) | ||
| result = _as_numpy(result) | ||
| assert "sklearnex" in neigh.__module__ | ||
| assert_allclose(result, [[2, 0]]) | ||
| result = neigh.kneighbors() |
There was a problem hiding this comment.
Just for my understanding: could you please explain why this is required after the assertion call?
There was a problem hiding this comment.
All use of kneighbors in test_neighbors passes a value to kwarg X. The datatype and queue are always extracted from it. However, that doesn't cover the case when the default X=None is used. This addition makes sure that the default use of kneighbors runs, though the output is not checked via an assert_allclose. This closes a testing gap.
There was a problem hiding this comment.
@icfaust thank you! I think make sense to add notes in comment section, since not that obvious in the code base.
Description
Calling k-NN algorithms
kneighbors()after fitting with GPU and algorithm = 'brute' will assume that it was fit using daal4py, and will fail in yielding kneighbors. This adds corrections in that check, and a test forkneighborswithout input.kneighborsis a bit special because it doesn't require an input to yield numerical results (re-using the fittedXvalues). The fit can occur in the daal4py backend or onedal backend. The check forpredictandkneighborsnow will check for which train object was generated, and will use it.Checklist to comply with before moving PR from draft:
PR completeness and readability
Testing
Performance