Migrate quantile.pxd to pylibcudf #15874
Conversation
lithomas1
added
feature request
github-actions
bot
added
Python
|
I dropped the memoryviews back in favor of directly typing as vector. I originally wanted to do a memoryview since I thought it'd be better performance wise, but now I realize that Cython can probably figure that out itself when doing the assignment (hopefully it's not going through the Python sequence protocol for that). |
There was a problem hiding this comment.
Regarding the quantiles argument and how we should type it, here's what I expect Cython will do:
- Typed memoryview: If the object in question exposes the buffer protocol, the function will receive a zero-copy view. If the object does not, it will error. This will work correctly for e.g. numpy arrays, but fail for lists/tuples/etc.
vector[double]: This will work for any iterable that contains doubles. Narrower types should be transparently upcasted, but that's worth testing (e.g. if you pass a tuple of ints, make sure that works).
So I think vector[double] is a good choice here. It should work fine from pure Python as well.
We seem to have lost the changes to the legacy _lib/quantiles.pyx though.
|
|
||
| Ignored if `is_input_sorted` is `Sorted.YES` | ||
| null_precedence: list, default None | ||
| A list of :py:class:`~cudf._lib.pylibcudf.types.NullOrder` enums, |
There was a problem hiding this comment.
Perhaps:
| A list of :py:class:`~cudf._lib.pylibcudf.types.NullOrder` enums, | |
| A list of :py:class:`~.types.NullOrder` enums, |
There was a problem hiding this comment.
nevermind, I don't think sphinx is able to find the enum as a class generally - I'm getting errors where sphinx isn't able to find the reference.
(maybe since the enum doesn't have a docstring).
I changed it back to regular backtics.
There was a problem hiding this comment.
We will definitely have to do some work on improving documentation of pylibcudf before the end. I'm hoping to find time between now and then to improve enum support in Cython in some of the edge cases that tend to bite us.
| @pytest.fixture(scope="module", params=[[1, 2, 3, 4, 5], [5, 4, 3, 2, 1]]) | ||
| def pa_col_data(request, numeric_pa_type): | ||
| return pa.array(request.param, type=numeric_pa_type) | ||
|
|
||
|
|
||
| @pytest.fixture(scope="module") | ||
| def plc_col_data(pa_col_data): | ||
| return plc.interop.from_arrow(pa_col_data) |
There was a problem hiding this comment.
Maybe combine these as we discussed in the other PR? Or you can wait for a follow-up, whatever you prefer.
There was a problem hiding this comment.
Thanks for the review, I'm planning on addressing in followup.
|
/merge |
Description
xref #15162
Migrate quantile.pxd to use pylibcudf APIs.
Checklist