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

Delete the deprecated data_type_id_t enum #4737

Merged
merged 7 commits into from
Oct 29, 2024

Conversation

ChuckHastings
Copy link
Collaborator

@ChuckHastings ChuckHastings commented Oct 23, 2024

Prep work for temporal graphs. We renamed data_type_id_t to cugraph_data_id_t about 8 months ago, this finishes the transition. We also split the types out into their own file in the C API.

Marked as breaking since we are deleting a deprecated type from the C API.

@ChuckHastings ChuckHastings added improvement Improvement / enhancement to an existing function breaking Breaking change and removed cuGraph python labels Oct 23, 2024
@ChuckHastings ChuckHastings marked this pull request as ready for review October 24, 2024 16:34
@ChuckHastings ChuckHastings requested review from a team as code owners October 24, 2024 16:34

typedef enum data_type_id_ {
INT32 = 0,
INT64,
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to add INT8, INT16, UINT8, UINT16, UINT32, UINT64?

These may become necessary in some algorithms in the future.

Copy link
Contributor

Choose a reason for hiding this comment

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

If we add these types but don't explicitly instantiate for these types, the cost is pretty minimal, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We can keep the cost minimal. I have added them in the next push.

Copy link
Contributor

@seunghwak seunghwak left a comment

Choose a reason for hiding this comment

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

Besides adding more integer types to be future proof, LGTM.

Copy link
Contributor

@rlratzel rlratzel left a comment

Choose a reason for hiding this comment

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

cython/python changes LGTM, CI should tell us if anything was missed.

@ChuckHastings
Copy link
Collaborator Author

/merge

@rapids-bot rapids-bot bot merged commit 3224a65 into rapidsai:branch-24.12 Oct 29, 2024
132 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Breaking change cuGraph improvement Improvement / enhancement to an existing function python
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants