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

[FEA] Cleanup on logic inside DeviceRolling::is_supported() #8399

Closed
nvdbaranec opened this issue May 28, 2021 · 4 comments · Fixed by #12503
Closed

[FEA] Cleanup on logic inside DeviceRolling::is_supported() #8399

nvdbaranec opened this issue May 28, 2021 · 4 comments · Fixed by #12503
Assignees
Labels
feature request New feature or request libcudf Affects libcudf (C++/CUDA) code.

Comments

@nvdbaranec
Copy link
Contributor

There is some logic in

static constexpr bool is_supported()
that really should be covered by is_valid_aggregation, but is_valid_aggregation is currently missing some edge cases.

 // MIN/MAX supports all fixed width types
 (((O == aggregation::MIN || O == aggregation::MAX) && cudf::is_fixed_width<T>()) ||

  // SUM supports all fixed width types except timestamps
  ((O == aggregation::SUM) && (cudf::is_fixed_width<T>() && !cudf::is_timestamp<T>())) ||

  // MEAN supports numeric and duration
  ((O == aggregation::MEAN) && (cudf::is_numeric<T>() || cudf::is_duration<T>())));

Once these edge cases are cleared up, clear out this code. Also, if is_valid_aggregation can handle a wide enough set of cases, it might be possible to remove the various is_supported() functions entirely and just have a single generic global template (checking is_valid_aggregation and has_corresponding_operator) used in it's place.

@nvdbaranec nvdbaranec added feature request New feature or request Needs Triage Need team to review and classify labels May 28, 2021
@nvdbaranec
Copy link
Contributor Author

Pinging @mythrocks

@beckernick beckernick added libcudf Affects libcudf (C++/CUDA) code. tech debt and removed Needs Triage Need team to review and classify labels Jul 12, 2021
@github-actions
Copy link

This issue has been labeled inactive-90d due to no recent activity in the past 90 days. Please close this issue if no further response or action is needed. Otherwise, please respond with a comment indicating any updates or changes to the original issue and/or confirm this issue still needs to be addressed.

@bdice
Copy link
Contributor

bdice commented Nov 28, 2022

@mythrocks @karthikeyann This came up in our meeting today. If you have any additional context on what needs to be done or whether this issue can be closed, feel free to share. The code has moved to a different filename since this issue was filed:

static constexpr bool is_supported()

@mythrocks
Copy link
Contributor

There is some cleanup that can be done here, but I don't know if there's enough to warrant a pair-up.
At the time, is_valid_aggregation() returned incorrect results for STRUCT/LIST. E.g. SUM on STRUCT columns. I haven't checked if that's been sorted out. If it has, then is_valid_aggregation() can be used in place of DeviceRolling::is_supported().

@GregoryKimball GregoryKimball moved this to Pairing in libcudf Jan 15, 2023
@rapids-bot rapids-bot bot closed this as completed in ed6daad Jan 24, 2023
GPUtester pushed a commit that referenced this issue Jan 24, 2023
This PR closes #8399. We cleaned up the logic by fixing SUM/MEAN aggregation type support, which also eliminated `TODO` comments in the target type definitions.

We kept the restriction that rolling min/max requires fixed width types because min/max aggregations do support non-fixed width in other aggregation implementations (groupby does a argmin-and-gather approach on strings, for instance).

This PR is collaborative work with @karthikeyann.

Authors:
  - Bradley Dice (https://github.com/bdice)
  - Karthikeyan (https://github.com/karthikeyann)

Approvers:
  - Mark Harris (https://github.com/harrism)
  - David Wendt (https://github.com/davidwendt)

URL: #12503
@GregoryKimball GregoryKimball removed the status in libcudf May 17, 2023
@GregoryKimball GregoryKimball removed this from libcudf Jun 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request New feature or request libcudf Affects libcudf (C++/CUDA) code.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants