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

Change exceptions thrown by copying APIs #15319

Merged
merged 5 commits into from
Mar 18, 2024

Conversation

vyasr
Copy link
Contributor

@vyasr vyasr commented Mar 15, 2024

Description

This PR also introduces std::out_of_range to cudf's code base in cases where it is appropriate.

Contributes to #12885
Resolves #15315
Contributes to #15162

Checklist

  • I am familiar with the Contributing Guidelines.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.

@vyasr vyasr added libcudf Affects libcudf (C++/CUDA) code. improvement Improvement / enhancement to an existing function breaking Breaking change labels Mar 15, 2024
@vyasr vyasr self-assigned this Mar 15, 2024
@vyasr vyasr requested a review from a team as a code owner March 15, 2024 16:39
@vyasr vyasr requested review from hyperbolic2346 and ttnghia March 15, 2024 16:39
@vyasr
Copy link
Contributor Author

vyasr commented Mar 15, 2024

Note to reviewers: let's discuss the usage of std::out_of_range and whether it's appropriate in #15315.

cpp/include/cudf/copying.hpp Outdated Show resolved Hide resolved
@@ -368,13 +368,16 @@ template <typename IndexIterator>
size_type validate_segmented_indices(IndexIterator indices_begin, IndexIterator indices_end)
{
auto const num_indices = static_cast<size_type>(std::distance(indices_begin, indices_end));
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be using thrust::distance instead of std::distance? I'm not sure if std::distance works on all iterator types. (I could be wrong here.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good question. I think it's worth making the change since I can see some code paths that call through to this with thrust iterators. I'm guessing everything works fine or we'd be seeing bugs, but best to be safe nonetheless.

I'll do this in a separate PR though.

cpp/src/copying/copy.cu Show resolved Hide resolved
cpp/src/copying/gather.cu Show resolved Hide resolved
@vyasr vyasr requested a review from bdice March 18, 2024 17:13
@vyasr
Copy link
Contributor Author

vyasr commented Mar 18, 2024

/merge

@rapids-bot rapids-bot bot merged commit 13cb4bc into rapidsai:branch-24.04 Mar 18, 2024
75 checks passed
@vyasr vyasr deleted the feat/copying_exceptions branch March 18, 2024 22:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Breaking change improvement Improvement / enhancement to an existing function libcudf Affects libcudf (C++/CUDA) code.
Projects
None yet
2 participants