First pass at adding testing for pylibcudf #15300
Conversation
vyasr
added
improvement
github-actions
bot
added
libcudf
| CUDF_EXPECTS(num_indices % 2 == 0, "Array of indices needs to have an even number of elements."); | ||
| CUDF_EXPECTS(num_indices % 2 == 0, | ||
| "Array of indices needs to have an even number of elements.", | ||
| std::invalid_argument); |
There was a problem hiding this comment.
issue: docstring now wants updated to match the actual return value.
cpp/src/copying/slice.cu
Outdated
| CUDF_EXPECTS(begin >= 0, "Starting index cannot be negative.", std::out_of_range); | ||
| CUDF_EXPECTS( | ||
| end >= begin, "End index cannot be smaller than the starting index.", std::invalid_argument); | ||
| CUDF_EXPECTS(end <= input.size(), "Slice range out of bounds.", std::out_of_range); |
There was a problem hiding this comment.
suggestion: although probably not as part of this PR. We should encode the validation mechanism for slice indices in one function, then all of the error messages would be the same, and if we decide the change our position on end <= input.size() (say) we could do it in one place.
github-actions
bot
removed
CMake
|
This PR is already getting quite large, so I decided to skip addressing nullable data here. We definitely need to do that, but we can do it in a follow-up, perhaps in @brandon-b-miller's PR adding tests for binops #15279 |
| # A simple wrapper around pytest.raises that defaults to looking for cudf exceptions | ||
| match = kwargs.get("match", None) | ||
| if match is None: | ||
| kwargs["match"] = "CUDF failure at" |
There was a problem hiding this comment.
Side note. CUDF being all caps here is kinda weird; should it be either libcudf or cuDF?
There was a problem hiding this comment.
That's a fair question, we could suggest that change to the libcudf team if we wanted. The text comes from https://github.com/rapidsai/cudf/blob/branch-24.06/cpp/include/cudf/utilities/error.hpp#L190 (and the corresponding CUDF_FAIL definition.
| return pytest.raises(expected_exception, *args, **kwargs) | ||
|
|
||
|
|
||
| # TODO: Consider moving these type utilities into pylibcudf.types itself. |
There was a problem hiding this comment.
We could also consider making them methods of plc.DataType
There was a problem hiding this comment.
Good point. Do you have a preference? And should we do that in a follow-up PR?
There was a problem hiding this comment.
Let's do it in a follow up. I'd prefer making them methods as pyarrow (and incidentally Polars) does that. But as Nick would say, it's a weakly held opinion
|
/merge |
Description
This PR adds tests of the
pylibcudf.copyingmodule along with establishing the infrastructure and best practices for writing pylibcudf tests going forward (and adding associated documentation).Resolves #15133
Checklist