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.

Sign up for GitHub

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

Jump to bottom

Expose stream parameter to public rolling APIs #15865

Merged
merged 4 commits into from
Jun 17, 2024
Merged

Expose stream parameter to public rolling APIs #15865

merged 4 commits into from
Jun 17, 2024

Conversation

srinivasyadav18
Copy link
Contributor

Description

Add stream parameter to public rolling APIs.

  • rolling()

  • grouped_rolling_window()

  • grouped_time_range_rolling_window()

  • grouped_range_rolling_window()

Checklist

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

@srinivasyadav18 srinivasyadav18 requested a review from a team as a code owner May 26, 2024 20:48
@copy-pr-bot copy-pr-bot
Copy link

copy-pr-bot bot commented May 26, 2024

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@github-actions github-actions bot added libcudf Affects libcudf (C++/CUDA) code. CMake CMake build issue labels May 26, 2024
@mhaseeb123
Copy link
Member

/okay to test

mythrocks
mythrocks approved these changes May 28, 2024
View reviewed changes
Copy link
Contributor

@mythrocks mythrocks left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Yep, LGTM. Thanks for making this change, @srinivasyadav18.

(Edit: Fixed the @.)

mhaseeb123
mhaseeb123 approved these changes May 28, 2024
View reviewed changes
Copy link
Member

@mhaseeb123 mhaseeb123 left a comment

Choose a reason for hiding this comment

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

LGTM!

@mhaseeb123 mhaseeb123 added improvement Improvement / enhancement to an existing function breaking Breaking change 3 - Ready for Review Ready for review by team labels May 28, 2024
@vyasr
Copy link
Contributor

vyasr commented Jun 3, 2024

@srinivasyadav18 could you take a look at the failing tests here?

@srinivasyadav18
Copy link
Contributor Author

Hi @vyasr . Yes, I found out what's causing the issue. The scalar's used by the grouped_rolling_window algorithm are not using the actual stream parameter, instead they are using the default stream. The issue is propogating from here. I will try to debug and fix it soon.

@vyasr
Copy link
Contributor

vyasr commented Jun 3, 2024

Nice find!

@srinivasyadav18
Expose stream parameter to public rolling APIs
af40eb7
@srinivasyadav18
update range_window_bounds API to accept stream parameter
88af8b4 
range_window_bounds API functions uses scalars internally, which should also the same stream parameter.

Signed-off-by: srinivasyadav18 <[email protected]>
@srinivasyadav18
Copy link
Contributor Author

/merge

@rapids-bot rapids-bot bot merged commit bcdfe91 into rapidsai:branch-24.08 Jun 17, 2024
75 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davidwendt davidwendt davidwendt left review comments

@mhaseeb123 mhaseeb123 mhaseeb123 approved these changes

@mythrocks mythrocks mythrocks approved these changes

Assignees
No one assigned
Labels
3 - Ready for Review Ready for review by team breaking Breaking change CMake CMake build issue improvement Improvement / enhancement to an existing function libcudf Affects libcudf (C++/CUDA) code.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@srinivasyadav18 @mhaseeb123 @vyasr @mythrocks @davidwendt