-
Notifications
You must be signed in to change notification settings - Fork 890
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
Add labeling APIs to pylibcudf #16761
Add labeling APIs to pylibcudf #16761
Conversation
ctypedef enum inclusive: | ||
YES "cudf::inclusive::YES" | ||
NO "cudf::inclusive::NO" | ||
cpdef enum class inclusive(bool): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this enum be exposed in the public API according to https://docs.rapids.ai/api/cudf/stable/developer_guide/pylibcudf/#cython-scoped-enums?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. Note also that the cudf::inclusive
enum has an underlying type of int
, not bool
.
cpdef enum class inclusive(bool): | |
cpdef enum class inclusive(int): |
Arguably that should be changed on the C++ side, although if we do that, we should change the order of the definition. So that bool(cudf::inclusive::YES)
is true.
ctypedef enum inclusive: | ||
YES "cudf::inclusive::YES" | ||
NO "cudf::inclusive::NO" | ||
cpdef enum class inclusive(bool): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. Note also that the cudf::inclusive
enum has an underlying type of int
, not bool
.
cpdef enum class inclusive(bool): | |
cpdef enum class inclusive(int): |
Arguably that should be changed on the C++ side, although if we do that, we should change the order of the definition. So that bool(cudf::inclusive::YES)
is true.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @mroeschke, lgtm!
Co-authored-by: Lawrence Mitchell <wence@gmx.li>
/merge |
Follow up to #16761, I forgot to add the doc pages for the labeling pylibcudf APIs Authors: - Matthew Roeschke (https://github.com/mroeschke) Approvers: - Matthew Murray (https://github.com/Matt711) URL: #16779
Description
Contributes to #15162
Checklist