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

C API for MNMG ECG #4194

Merged
merged 12 commits into from
Mar 7, 2024
Merged

C API for MNMG ECG #4194

merged 12 commits into from
Mar 7, 2024

Conversation

naimnv
Copy link
Contributor

@naimnv naimnv commented Feb 26, 2024

C API for MNMG ECG

Closes #3571
Closes #3572
Closes #3573
Closes #3575

@naimnv naimnv requested review from a team as code owners February 26, 2024 22:13
Copy link

copy-pr-bot bot commented Feb 26, 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.

@naimnv naimnv self-assigned this Feb 26, 2024
@naimnv naimnv added this to the 24.04 milestone Feb 26, 2024
@ChuckHastings ChuckHastings added improvement Improvement / enhancement to an existing function breaking Breaking change labels Feb 26, 2024
@ChuckHastings
Copy link
Collaborator

Labeling a BREAKING change, although the breakage is the renaming of the legacy C API implementation, it is not expected that there are any current users of the legacy C API other than the python layer which is corrected in this PR.

@naimnv
Copy link
Contributor Author

naimnv commented Feb 26, 2024

/okay to test

Copy link
Contributor

@jnke2016 jnke2016 left a comment

Choose a reason for hiding this comment

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

The PR looks good overall. Just few minor comments.

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.

LGTM except for the comments from Joseph.

cpp/tests/c_api/ecg_test.c Show resolved Hide resolved
@naimnv
Copy link
Contributor Author

naimnv commented Feb 29, 2024

/okay to test

1 similar comment
@naimnv
Copy link
Contributor Author

naimnv commented Feb 29, 2024

/okay to test

Copy link
Contributor

@jnke2016 jnke2016 left a comment

Choose a reason for hiding this comment

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

Thanks for addressing those minor change requests. Looks good to me

@naimnv
Copy link
Contributor Author

naimnv commented Feb 29, 2024

/okay to test

@naimnv
Copy link
Contributor Author

naimnv commented Feb 29, 2024

/okay to test

@naimnv
Copy link
Contributor Author

naimnv commented Feb 29, 2024

/okay to test

@naimnv
Copy link
Contributor Author

naimnv commented Mar 7, 2024

/okay to test

@BradReesWork
Copy link
Member

/merge

@rapids-bot rapids-bot bot merged commit 72401a0 into rapidsai:branch-24.04 Mar 7, 2024
136 checks passed
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.

Sorry for the late review. This LGTM.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Breaking change CMake cuGraph improvement Improvement / enhancement to an existing function python
Projects
None yet
6 participants