Skip to content

Fork out percentile_dispatch to dask.array#8083

Merged
jrbourbeau merged 15 commits intodask:mainfrom
galipremsagar:percentile_re
Aug 25, 2021
Merged

Fork out percentile_dispatch to dask.array#8083
jrbourbeau merged 15 commits intodask:mainfrom
galipremsagar:percentile_re

Conversation

@galipremsagar
Copy link
Copy Markdown
Contributor

@galipremsagar galipremsagar commented Aug 25, 2021

  • Closes #xxxx
  • Tests added / passed
  • Passes black dask / flake8 dask / isort dask

@github-actions github-actions bot added array dataframe dispatch Related to `Dispatch` extension objects labels Aug 25, 2021
Copy link
Copy Markdown
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.

Thanks for the quick fix @galipremsagar! Overall this looks good. I've left a few small comments below

Comment on lines +17 to +18
def _percentile(a, q, interpolation="linear"):
return percentile_dispatch(a, q, interpolation)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is this needed? Can we just rely on percentile_dispatch.__call__ instead?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, removed this. Is there a plan to replace this kind of dispatch invocation to other dispatches? Not in scope of PR but wanted to know.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

No concrete plan, but if we're able to safely remove similar wrapper functions, then I'd be +1

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Noted.

@jrbourbeau
Copy link
Copy Markdown
Member

Thanks for all the updates @galipremsagar, they look great!

It looks like the gpuCI build isn't happy with some of the changes here. We're getting error s like TypeError: No dispatch for <class 'cudf.core.series.Series'>. I suspect that we'll just need to update to use the new percentile dispatch object here and gpuCI will pass, but it would be good if you could confirm that.

@galipremsagar
Copy link
Copy Markdown
Contributor Author

Thanks for all the updates @galipremsagar, they look great!

It looks like the gpuCI build isn't happy with some of the changes here. We're getting error s like TypeError: No dispatch for <class 'cudf.core.series.Series'>. I suspect that we'll just need to update to use the new percentile dispatch object here and gpuCI will pass, but it would be good if you could confirm that.

Yeah, I can make the changes on cudf side only once this PR is merged. So lets merge this if everything other than gpuCI passes.

Copy link
Copy Markdown
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.

Sounds good to me 👍 Will merge once CI finishes. Thanks again for handling this.

@jrbourbeau jrbourbeau merged commit 7cfee4a into dask:main Aug 25, 2021
rapids-bot bot pushed a commit to rapidsai/cudf that referenced this pull request Aug 26, 2021
This PR adds backend for `percentile_lookup` dispatch in `dask_cudf`, related dask upstream changes were done in dask/dask#8083

Authors:
  - GALI PREM SAGAR (https://github.com/galipremsagar)

Approvers:
  - https://github.com/jakirkham

URL: #9118
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

array dataframe dispatch Related to `Dispatch` extension objects

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants