search

soda-inria / sklearn-numba-dpex

Experimental plugin for scikit-learn to be able to run (some estimators) on Intel GPUs via numba-dpex.
BSD 3-Clause "New" or "Revised" License
15 stars 4 forks source link

FEAT implement sum over axis0 #80

Closed fcharras closed 1 year ago

fcharras commented 1 year ago

Follows https://github.com/soda-inria/sklearn-numba-dpex/pull/79

Implements sum over axis 0

It seems that this kernel will be necessary for implementation of topK.

jjerphan commented 1 year ago

FYI: I am waiting for #79 to be merged before reviewing this PR.

fcharras commented 1 year ago

Ready for review

fcharras commented 1 year ago

TY for the reviews @jjerphan @ogrisel I've applied your suggestions (in particular: removed the recursive call) and answered the questions for everything except unit tests, which will come in a future commit.

fcharras commented 1 year ago

Added tests for edge cases, and for invalid work group or sub group sizes.

fcharras commented 1 year ago

Latest commit aims at addressing the remaining comments.

fcharras commented 1 year ago

Why does the test start failing only now ? I guess it would be related to https://github.com/soda-inria/sklearn-numba-dpex/pull/85 that has been merged but this branch doesn't include the change yet ? :thinking:

jjerphan commented 1 year ago

@fcharras, should we merge main in the branch of this PR?

fcharras commented 1 year ago

Yes, if what's happening is that github runs the tests after merging on main (?), main needs to be merged first so the test can be adjusted.

fcharras commented 1 year ago

I'll do it, but was waiting for @ogrisel to finish review.

ogrisel commented 1 year ago

Feel free to merge main to this branch when you want.

fcharras commented 1 year ago

Thanks for review ! That would be really cool if you can open follow-up PRs. :+1: