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

Update libcudf counting functions to specify cudf::size_type #12904

Merged
merged 14 commits into from
Mar 15, 2023
Merged

Update libcudf counting functions to specify cudf::size_type #12904

merged 14 commits into from
Mar 15, 2023

Conversation

davidwendt
Copy link
Contributor

@davidwendt davidwendt commented Mar 8, 2023 •
edited
Loading

Description

Adds section to developer guide about cudf::size_type and adds links to it from other relevant parts of the document.
The fundamental nature of this type seems important enough to mention in the developer guide since it is the basis for how much of the code is designed and implemented.
Also updates some doxygen for public APIs that are return size_type column values but had cited INT32 specifically.

Reference: #12779 (comment)

Checklist

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

@davidwendt davidwendt added 2 - In Progress Currently a work in progress libcudf Affects libcudf (C++/CUDA) code. tech debt improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Mar 8, 2023
@davidwendt davidwendt self-assigned this Mar 8, 2023
@davidwendt davidwendt mentioned this pull request Mar 8, 2023
Merged
3 tasks
@davidwendt davidwendt added 3 - Ready for Review Ready for review by team and removed 2 - In Progress Currently a work in progress labels Mar 9, 2023
@davidwendt davidwendt marked this pull request as ready for review March 9, 2023 16:33
@davidwendt davidwendt requested a review from a team as a code owner March 9, 2023 16:33
bdice
bdice reviewed Mar 9, 2023
View reviewed changes
Copy link
Contributor

@bdice bdice left a comment

Choose a reason for hiding this comment

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

Just a few minor comments. This is a significant improvement in clarity for our library design. Thank you very much!

cpp/doxygen/developer_guide/DEVELOPER_GUIDE.md Outdated Show resolved Hide resolved
cpp/doxygen/developer_guide/DEVELOPER_GUIDE.md Outdated Show resolved Hide resolved
cpp/include/cudf/strings/attributes.hpp Show resolved Hide resolved
cpp/include/nvtext/tokenize.hpp Outdated Show resolved Hide resolved
rapids-bot bot pushed a commit that referenced this pull request Mar 10, 2023
@davidwendt
Remove return type from @return doxygen tags (#12908)
e37bddb 
The doxygen `@return` does not require a type but only the description. However, the doxygen output looks cleaner without the type. There were only a few places in the public header files that needed to be corrected.
This technically a documentation-only change.
Found when working on #12904

Authors:
  - David Wendt (https://github.com/davidwendt)

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

URL: #12908
karthikeyann
karthikeyann reviewed Mar 13, 2023
View reviewed changes
Copy link
Contributor

@karthikeyann karthikeyann left a comment

Choose a reason for hiding this comment

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

At some @return doc, size_type is removed, but left cudf::size_type in some @brief.
What is the convention followed here?

wence-
wence- reviewed Mar 14, 2023
View reviewed changes
Copy link
Contributor

@wence- wence- left a comment

Choose a reason for hiding this comment

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

One grammar change (while we're changing the line anyway)

cpp/doxygen/developer_guide/DEVELOPER_GUIDE.md Outdated Show resolved Hide resolved
@davidwendt
Copy link
Contributor Author

I believe all comments have been addressed now.

wence-
wence- approved these changes Mar 14, 2023
View reviewed changes
Copy link
Contributor

@wence- wence- left a comment

Choose a reason for hiding this comment

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

My approval carries no official weight here I think, but looks good, thanks!

@davidwendt
Copy link
Contributor Author

/merge

@rapids-bot rapids-bot bot merged commit 7776e0e into rapidsai:branch-23.04 Mar 15, 2023
@davidwendt davidwendt deleted the doc-size-type branch March 15, 2023 14:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bdice bdice bdice left review comments

@PointKernel PointKernel PointKernel approved these changes

@ttnghia ttnghia ttnghia approved these changes

@karthikeyann karthikeyann karthikeyann approved these changes

@wence- wence- wence- approved these changes

Assignees

@davidwendt davidwendt

Labels
3 - Ready for Review Ready for review by team improvement Improvement / enhancement to an existing function libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@davidwendt @wence- @bdice @karthikeyann @ttnghia @PointKernel