Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Adjust all algorithms to work with CuPy #75

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

@pentschev
Copy link
Member Author

The build will fail now, since it depends on NumPy and CuPy fixes that weren't yet merged. However, I can confirm that all existing tests pass on my setup after the changes here.

Copy link
Member

@mrocklin mrocklin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @pentschev !

We'll probably have to be a bit careful around where we import cupy to make sure that this doesn't affect normal maintenance and usage of this library.

Also, I'm surprised that there weren't any code changes. All I'm seeing here is testing code (which all looks quite nice). Did you have to modify any code to get things to work?

dask_glm/utils.py Outdated Show resolved Hide resolved
dask_glm/tests/cupy/test_regularizers.py Show resolved Hide resolved
dask_glm/utils.py Outdated Show resolved Hide resolved
Copy link
Member

@jrbourbeau jrbourbeau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On a related note, the estimators in dask-glm were implemented in dask-ml (ref dask/dask-ml#94). I think the plan was to deprecate dask_glm.estimators (#66) but keep the optimizers and regularizers in dask-glm.

@TomAugspurger is this still the case?

@TomAugspurger
Copy link
Member

TomAugspurger commented Mar 8, 2019 via email

@pentschev
Copy link
Member Author

FWIW, I have no strong opinions there.

Sorry if this question has already been answered before or sounds a bit stupid, but wouldn't it then make sense to have Dask-GLM completely deprecated and move everything into Dask-ML? GLM being a statistical model, it doesn't fall too far off of ML, but of course, maybe syntactically it could be considered something very unrelated.

@jrbourbeau
Copy link
Member

My personal preference would be to merge #66 and deprecate dask_glm.estimators in favor of dask_ml.linear_model. That way all the scikit-learn style estimators live in one place.

@TomAugspurger
Copy link
Member

TomAugspurger commented Mar 8, 2019 via email

@matthieubulte
Copy link

Hi, this PR would be quite useful for me.

Are you only waiting for #66 to be finalized and merged to merge this one?

It seems that #66 is paused for now, is that something where I could help to unblock this PR?

Otherwise, anything else I could do to get this merged?

Thanks!

@pentschev
Copy link
Member Author

As far as I recall, this PR should mostly work (if not entirely). It's not necessarily dependant upon #66, even though a little modification would be required once it's merged. However, the changes here are dependant upon several unreleased changes:

  1. ENH: Add shape to *_like() array creation numpy/numpy#13046 -- will be available in NumPy 1.17
  2. Support for shape argument in *_like functions cupy/cupy#2171 -- will only be merged once NumPy 1.17 is released, then available on the next CuPy release after that
  3. Allow copying in the format cupy_array[:] = numpy_array cupy/cupy#2079 / [backport] Allow copying in the format cupy_array[:] = numpy_array cupy/cupy#2219 -- available in CuPy 7.0.0b1, but requires setting CUPY_EXPERIMENTAL_SLICE_COPY=1 environment variable

That said, we are likely to wait to have all these changes released before we can merge that. I'm expecting that NumPy is released sometime this month, following that, probably a CuPy release with necessary changes is probably due in August or September.

@matthieubulte
Copy link

Great, thanks for the clarifications!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

NEP-18: CuPy backend requires unimplemented algorithms
5 participants