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

Refactor chevron to cub #2069

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Refactor chevron to cub #2069

wants to merge 2 commits into from

Conversation

ZelboK
Copy link
Contributor

@ZelboK ZelboK commented Jul 24, 2024

I had a PR here
https://github.com/NVIDIA/cccl/pull/592/files that I forgot about. It would probably easier to just make a new PR instead of trying to rebase that one since it's old.

With that being said to ease burden on the reviewer I've only touched 1 file(for now) and left comments where I am asking questions. If this work isn't needed anymore LMK and I will close both PRs.

Checklist

  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.

@ZelboK ZelboK requested review from a team as code owners July 24, 2024 23:40
@ZelboK ZelboK requested review from elstehle and alliepiper July 24, 2024 23:40
Copy link

copy-pr-bot bot commented Jul 24, 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.

@ZelboK ZelboK marked this pull request as draft July 24, 2024 23:45
@ZelboK ZelboK marked this pull request as ready for review July 24, 2024 23:52
CUB_NAMESPACE_BEGIN


namespace detail
Copy link
Contributor Author

Choose a reason for hiding this comment

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

is this namespace cocrrect? i see there's cuda_cub in the thrust launcher

@bernhardmgruber
Copy link
Contributor

bernhardmgruber commented Jul 25, 2024

I had a PR here
https://github.com/NVIDIA/cccl/pull/592/files that I forgot about. It would probably easier to just make a new PR instead of trying to rebase that one since it's old.

Those are independent things. You could create a new branch locally (with a different name) and force push it to the same branch in your fork, from which the PR was created. You could also just git reset --hard your local branch to our upstream main, start over, and force push. The best thing would still be to rebase your changes. Try to let a good editor/IDE help you. I am sure VSCode has a good merge viewer.

By creating a new PR, you create discontinuity in the review, especially because @gevtushenko already provided a lot of feedback on your old PR, some of which is unaddressed.

@bernhardmgruber
Copy link
Contributor

Independently on where you want to continue your work, this PR seems to add a copy of Thrust's trivple_chevron into CUB. #568 seems to suggest to move the functionality instead, so it should no longer exist in Thrust.

@ZelboK
Copy link
Contributor Author

ZelboK commented Jul 25, 2024

Independently on where you want to continue your work, this PR seems to add a copy of Thrust's trivple_chevron into CUB. #568 seems to suggest to move the functionality instead, so it should no longer exist in Thrust.

Yes, I did not remove Thrust's chevron here. That would be the next step. But I wasn't sure if this work was still required, so I decided to ask questions here first. Before progressing further I wanted to make sure this was still needed so I could avoid wasting my time in the case it wasn't. Sorry for not clarifying on that

@bernhardmgruber
Copy link
Contributor

I wasn't sure if this work was still required, so I decided to ask questions here first.

This work seems similar to #2038, so maybe @pciolkosz knows the answer to that.

@ZelboK
Copy link
Contributor Author

ZelboK commented Jul 25, 2024

I wasn't sure if this work was still required, so I decided to ask questions here first.

This work seems similar to #2038, so maybe @pciolkosz knows the answer to that.

Thank you. I'll hold off on this one then for now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In Review
Development

Successfully merging this pull request may close these issues.

2 participants